[FIRRTL] Fix sub-* op in layer block verifier #7462
Conversation
There was a problem hiding this comment.
LGTM!
The language about "passive captures" threw me a bit, looks like that's dropped which makes sense to me!
This sort of thing is tricky to do appropriately generically presently (if rvalues were separate from lvalues, that sort of thing) but I think this looks right to me!
| // We can drive any destination inside the current layerblock. | ||
| // Verify that connects only drive values declared in the layer block. | ||
| // If we see a non-passive connect destination, then verify that the | ||
| // source is in the same layer block so that the source is not driven. |
There was a problem hiding this comment.
Thanks for this comment explaining why the passivity of the operand is being checked!
(and great explanation in PR body, 🙇 )
I was going to file issues but I'll leave these examples here (and less sure about them, WDYT?) -- they seem like they ... could be supported? It depends on whether driving a bundle with only flip elements (similarly a bundle with zero-width but flipped element) counts as violating the rules re:layer.
Non-passive but only leaf connection is into the layer:
FIRRTL version 4.0.0
circuit LF:
layer T, bind :
public module LF:
output o : { flip x : UInt<1> }
layerblock T :
wire w : { flip x : UInt<1> }
connect o, wSimilarly:
FIRRTL version 4.0.0
circuit LF:
layer L, bind :
type T = { flip a : { flip b : UInt<5> } }
extmodule Sink:
input in : UInt<5>
public module LF:
input x : T
layerblock L:
inst s of Sink
connect s.in, x.a.bAnyway looks like the "passive capture" rule is an verifier optimization to avoid having to chase/check too many operations in the common case.
Gosh, having this reliably sorted would really make this simpler -- really don't want to have to teach this verifier how to understand "connect" in all it's glory.
Anyway it's arguable that connecting bundle isn't the same as connecting its elements (the framing here changes AFAICT in different contexts) in which case these should be rejected.
Onwards!
There was a problem hiding this comment.
I think the PR is just wrong. I had started to write his using foldFlow which I think is how it has to work if the operation is a connect. The check for matchingconnect and simpler operations is easier. Let me dig into this.
There was a problem hiding this comment.
foldFlow isn't quite right either. In order for this to work correctly, it needs to know if something is a source, sink, or duplex. However, this isn't the usual foldFlow computation which looks up towards the declaration, but looks down towards the leaves of the type.
It's annoying because I think it would be just a passive check if type canonicalization was possible.
There was a problem hiding this comment.
This is exceedingly annoying to verify... It requires examining the type in detail. In this PR, I opted to not verify this. I don't like this, however, non-passive types are incredibly short-lived in the compiler. These are almost immediately removed due to everything becoming matchingconnect. This saves some tedious, expensive verification for a code path that is pretty rare.
I put a massive TODO in the code explaining this and will expect to revisit this in the future.
I'd like to merge this as-is and we can tighten this up later.
lib/Dialect/FIRRTL/FIRRTLOps.cpp
Outdated
| if (isAncestor(getOperation(), dest)) { | ||
| if (passive) | ||
| return WalkResult::advance(); | ||
| dest = getFieldRefFromValue(connect.getSrc()).getValue(); |
There was a problem hiding this comment.
These may want to look through casts to chase to the actual defining op? Maybe future work.
There was a problem hiding this comment.
Agree. I'll put this in the future work bucket.
seldridge
force-pushed
the
dev/seldridge/issue-7451
branch
2 times, most recently
from
eaaad49 to
fcb92a0
Compare
seldridge
requested review from
dtzSiFive and
darthscsi
and removed request for
darthscsi
Fix issues with verification of subfield, subindex, and subaccess operations which appear in a layer block. These operations are allowed to occur in a layer block even if they capture non-passive operands. This requires reworking layer block verification to no longer check for operations using non-passive operands. The spec requires that no operation in a layer block _drives_ a value declared outside the layer block. However, this is exceedingly difficult to verify due to the fact that non-passive destinations in ConnectLike operations can be source-to-destination, destination-to-source, or bi-directional. If the verifier sees this, just allow it. The FIRRTL pass pipeline will later canonicalize away flips (i.e., make all types passive) which will then allow the verifier to check these. This should be revisited in the future. Fixes #7451. Signed-off-by: Schuyler Eldridge <[email protected]> Don't check non-passive connects.
seldridge
force-pushed
the
dev/seldridge/issue-7451
branch
from
fcb92a0 to
a6028ee
Compare
Fix issues with verification of subfield, subindex, and subaccess
operations which appear in a layer block. These operations are allowed to
occur in a layer block even if they capture non-passive operands.
This requires reworking layer block verification to no longer check for
operations using non-passive operands. The spec requires that no
operation in a layer block drives a value declared outside the layer
block. However, this is exceedingly difficult to verify due to the fact
that non-passive destinations in ConnectLike operations can be
source-to-destination, destination-to-source, or bi-directional. If the
verifier sees this, just allow it. The FIRRTL pass pipeline will later
canonicalize away flips (i.e., make all types passive) which will then
allow the verifier to check these. This should be revisited in the
future.
Fixes #7451.
Metadata
The commits are in logical order and can be reviewed independently.