[LLHD] Add combinational process op #8536
Conversation
| } | ||
| } | ||
| ``` | ||
| }]; |
There was a problem hiding this comment.
Are there any restrictions around drives in this op? Multiple drives to the same signal (resulting in conflicts if naively inlined)? Conditional drives? Essentially, whose responsibility is it to check for such things, the pass that creates this op or the pass that analyses/lowers this op?
There was a problem hiding this comment.
I think you can have anything you'd want in here. Might sometimes even be desirable, for example, if we want to directly map from Verilog's always_comb to llhd.combinational. That would then still contain weird drives. I think the difference is that a drive in a process/combinational op has different semantics from a drive in the module -- the former is like a store, overwriting previous values, while the latter is a permanent driver that produces drive conflicts. We might want to split these into separate ops in the future, to make it clear that moving drives from processes into modules is dangerous. It would then be the pass lowering/inlining the combinational op that has to check if the inlining is allowed. For example, if it contains any procedural drives it can't be inlined into a graph-region parent.
There was a problem hiding this comment.
That sounds like a reasonable decision. Properly verifying any of these restrictions/invariants would be almost impossible in practice anyway. llhd.wait terminators are not allowed in this op though, right?
There was a problem hiding this comment.
Yes exactly! With the llhd.yield, you can only return a bunch of values as the process results. No suspension of execution is possible. So I guess most of the weirdness of processes disappears through this 🤔
| cf.cond_br %arg0, ^bb1(%arg1, %arg2 : i42, i9001), ^bb1(%arg3, %arg4 : i42, i9001) | ||
| ^bb1(%1: i42, %2: i9001): | ||
| llhd.halt %1, %2 : i42, i9001 | ||
| } |
There was a problem hiding this comment.
No CHECK directives to verify detail of the printer that the parser doesn't necessarily care about (e.g. whitespace)?
There was a problem hiding this comment.
I was hoping for verify-roundtrip to do a reasonable check, maybe minus the whitespace 😇
There was a problem hiding this comment.
I had messed up whitespace in the assembly format before (especially around parentheses). It didn't break functionality but was weird to look at. That's why I brought it up.
fabianschuiki
force-pushed
the
fschuiki/comb-op
branch
from
2d39e1c to
3c567f2
Compare
fabianschuiki
force-pushed
the
fschuiki/comb-op
branch
2 times, most recently
from
b53a013 to
b01930c
Compare
| } | ||
| } | ||
| ``` | ||
| }]; |
There was a problem hiding this comment.
That sounds like a reasonable decision. Properly verifying any of these restrictions/invariants would be almost impossible in practice anyway. llhd.wait terminators are not allowed in this op though, right?
| cf.cond_br %arg0, ^bb1(%arg1, %arg2 : i42, i9001), ^bb1(%arg3, %arg4 : i42, i9001) | ||
| ^bb1(%1: i42, %2: i9001): | ||
| llhd.halt %1, %2 : i42, i9001 | ||
| } |
There was a problem hiding this comment.
I had messed up whitespace in the assembly format before (especially around parentheses). It didn't break functionality but was weird to look at. That's why I brought it up.
Add a dedicated op for processes that should trigger when any of the values used as operands in its body region change. This reflects the behavior of `always_comb` in SystemVerilog and will allow passes such as desequentialization and process lowering to convert `llhd.process`es to this more streamlined and specialized representation. A later commit will replace uses of `scf.execute_region` in the LLHD pass pipeline with `llhd.combinational`.
fabianschuiki
force-pushed
the
fschuiki/comb-op
branch
from
b01930c to
a3acd0c
Compare
Add a dedicated op for processes that should trigger when any of the values used as operands in its body region change. This reflects the behavior of `always_comb` in SystemVerilog and will allow passes such as desequentialization and process lowering to convert `llhd.process`es to this more streamlined and specialized representation. A later commit will replace uses of `scf.execute_region` in the LLHD pass pipeline with `llhd.combinational`.
Add a dedicated op for processes that should trigger when any of the values used as operands in its body region change. This reflects the behavior of
always_combin SystemVerilog and will allow passes such as desequentialization and process lowering to convertllhd.processes to this more streamlined and specialized representation.A later commit will replace uses of
scf.execute_regionin the LLHD pass pipeline withllhd.combinational.