[LLHD] Add support for llhd.sig.array_get projections to Mem2Reg
#8509
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
maerhart
approved these changes
May 23, 2025
| llhd.halt | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Maybe add tests for the following scenarios:
- Projected signal value is passed via block argument within a process (make sure it doesn't crash)
- Multiple projection ops refer to the same part of the original signal (make sure multiple drives to these different projections that refer to the same part update to the correct value)
There was a problem hiding this comment.
Ooops good idea, that's definitely missing from the tests. Let me add that.
| // as any of their users. | ||
| if (isa<SigArrayGetOp>(user)) { | ||
| for (auto *projectionUser : user->getUsers()) { | ||
| if (projectionUser->getBlock() != user->getBlock()) |
There was a problem hiding this comment.
Isn't that too restrictive for many practical cases due to CSE of projection ops? (not blocking)
There was a problem hiding this comment.
Yes that's a good point 👍. We should be able to relax this by only requiring that the projection op dominates its users. That would also allow projections in dominating predecessor blocks and in the module to be covered, but still allow us to not reason about signals references through block arguments (that'll be a mighty headache).
1d733ad to
a59ef60
Compare
Extend the Mem2Reg pass to support drives to `llhd.sig.array_get`
projections into a promotable signal. This requires a few changes:
- When checking whether a signal is promotable, we have to recursively
check its users and look through `llhd.sig.array_get` ops to see if
all uses are safe for promotion.
- The lattice used to propagate needed and reaching definitions has to
track the projection being driven separately from the underlying root
slot being driven.
- A new set of helpers to unpack and pack stacks of projections help
probes and drives take a promotable slot and its value, descend into
subfields as indicated by a set of projection ops ("unpack"), and then
optionally mutate the value of a subfield and zip the updated value
back up to the promotable slot ("pack").
- During backward and forward propagation across the lattice, drives to
projections have to create the need for a definition of the slot to be
available because they have to take the slot's current value in order
to mutate the projected field.
- The functions that resolve definitions have to unpack a slot's value
according to the projections between the probe and the slot. In case
of a drive they also have to mutate the projected field value after
unpacking and then pack everything back op in order to create a new
reaching definition for the entire slot to be forwarded to readers.
This commit also drops `slotOrder` in favor of a `promotable` set of
values. The actual order of slots was never used.
a59ef60 to
f8bba03
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Extend the Mem2Reg pass to support drives to
llhd.sig.array_getprojections into a promotable signal. This requires a few changes:When checking whether a signal is promotable, we have to recursively check its users and look through
llhd.sig.array_getops to see if all uses are safe for promotion.The lattice used to propagate needed and reaching definitions has to track the projection being driven separately from the underlying root slot being driven.
A new set of helpers to unpack and pack stacks of projections help probes and drives take a promotable slot and its value, descend into subfields as indicated by a set of projection ops ("unpack"), and then optionally mutate the value of a subfield and zip the updated value back up to the promotable slot ("pack").
During backward and forward propagation across the lattice, drives to projections have to create the need for a definition of the slot to be available because they have to take the slot's current value in order to mutate the projected field.
The functions that resolve definitions have to unpack a slot's value according to the projections between the probe and the slot. In case of a drive they also have to mutate the projected field value after unpacking and then pack everything back op in order to create a new reaching definition for the entire slot to be forwarded to readers.
This commit also drops
slotOrderin favor of apromotableset of values. The actual order of slots was never used.