[FIRRTL] Add a new pass to detect static asserts #6341
Conversation
prithayan
requested review from
darthscsi,
dtzSiFive and
seldridge
as code owners
| auto fModule = getOperation(); | ||
| (void)failableParallelForEach( | ||
| fModule.getContext(), fModule.getOps(), | ||
| [&](Operation &op) { return checkAssert(&op); }); |
There was a problem hiding this comment.
If emitting an error, this should fail the pass.
There was a problem hiding this comment.
Yes, missed it. Added a failure, but we can discuss if we want this to be a failure or just a warning.
| }; | ||
| LogicalResult checkAssert(Operation *op) { | ||
| Value predicate; | ||
| if (auto a = dyn_cast<AssertOp>(op)) { |
There was a problem hiding this comment.
Should this be an actual analysis that is inspected by the pass, where diagnostics are emitted?
Fine as-is, but consider especially as this grows.
There was a problem hiding this comment.
Right, this was simple enough, but we could consider it if an analysis is required for other uses.
| let summary = "An analysis pass to detect static asserts."; | ||
| let description = [{ | ||
| This pass detects assert ops, whose predicate condition can be statically | ||
| inferred to be false, that is they will trivially fail any simulation. The |
There was a problem hiding this comment.
Should this have a (slightly, for now) more general description?
It seems like presently this already does more than "statically inferred to be false" -- reset signal probably will be false at some point (diagnostic sounds useful) but is it fair to summarize as statically inferred to be false?
Anyway either commit to maintaining this or maybe give you a little wiggle room re:description 👍.
There was a problem hiding this comment.
Agree, updated the description.
| void runOnOperation() override { | ||
| auto fModule = getOperation(); | ||
| (void)failableParallelForEach( | ||
| fModule.getContext(), fModule.getOps(), |
There was a problem hiding this comment.
I'm not sure if parallel over operations is a good idea? Thread-to-work ratio seems a bit rough, but dunno.
Misc potential preconditions/assumptions:
- post ExpandWhen's / single-block
- IMDCE already executed (in theory if this was run before such, what if the assert is not reachable/dead?).
That sound about right? Just trying to understand this and it's role/purpose/scope.
And pipeline-ordering-wise probably of course want to run this after various optimizations such that if our compiler can/will prove these are statically false/lint-worthy, we'll have done so (e.g., IMCP).
There was a problem hiding this comment.
You are right, parallel work seems very small. Removed it.
It is expected that all the optimizations are done, additionally post expand-whens, hence running it late in the pipeline.
I will update the comments to indicate the assumptions.
prithayan
force-pushed
the
dev/pbarua/assert_verif
branch
from
175480c
to
1987f3b
Compare
|
Thanks for the review everyone. It would be nice to keep track of other lint checks we can add. |
|
Could this be a home for @darthscsi's analysis that checks if uninitialized data leaks into registers? |
This PR adds a new pass called
Lintto theFIRRTLpipeline, that checks for asserts that are guaranteed to fail a simulation.If the assert is enabled and the predicate is either a constant false or a reset, the pass emits an error on the assert.
The intention is that the pass can later be extended to include more checks.