Add ifElseFatal intrinsics; use it for chisel3.assert emission #4001
Conversation
dtzSiFive
added
the
Backend Code Generation
Restore chipsalliance#3912 and chipsalliance#3825. Use intrinsic expression instead of intrinsic module. This reverts commit 347d82b.
dtzSiFive
force-pushed
the
feature/verif-intrinsic-again
branch
from
520ab47
to
e45a9e0
Compare
| { | ||
| val x5 = { | ||
| val x1 = chisel3.assert(1.U === 1.U) | ||
| val x2 = cover(foo =/= bar) | ||
| val x3 = chisel3.assume(foo =/= 123.U) | ||
| val x4 = printf("foo = %d\n", foo) | ||
| x1 | ||
| x2 | ||
| } | ||
| } | ||
| } | ||
| val chirrtl = ChiselStage.emitCHIRRTL(new Test) | ||
| (chirrtl should include).regex("assert.*: x5") | ||
| (chirrtl should include).regex("cover.*: x5_x2") | ||
| (chirrtl should include).regex("cover.*: x5") | ||
| (chirrtl should include).regex("assume.*: x5_x3") |
There was a problem hiding this comment.
I know this PR is just a re-application of 2 previously reverted PRs so I'm not going to block on this issue (since it was in a PR I previously approved), but this is not ideal. We should be able to maintain the old label behavior, but it will probably require a new Printable that wraps the Assert() object that we return from chisel3.assert.
There was a problem hiding this comment.
Ah, so instead of "chisel3_builtin" being the label for all of these, they get the "valname"/name included that would previously be mixed in by putting it in the "optional name" (trailing ": asdf" this test checks for)?
Yes that seems rather useful!
So Printable would help get a friendly name available (that's not available when IntrinsicExpr apply is called) so we could append it to the label StringParam while converting to the FIRRTL IR object, thereabouts? Makes sense to me!
There was a problem hiding this comment.
FYI previous behavior was just dropping optional names (assert_ label in the test) emitted for assert op. chisel3_builtin was used regardless of optional names.
There was a problem hiding this comment.
So Printable would help get a friendly name available (that's not available when IntrinsicExpr apply is called) so we could append it to the label StringParam while converting to the FIRRTL IR object, thereabouts? Makes sense to me!
Exactly!
FYI previous behavior was just dropping optional names (assert_ label in the test) emitted for assert op. chisel3_builtin was used regardless of optional names.
Perhaps for these asserts in firtool, but the intent was always to try to replace printf-encoded with real asserts with reasonable label behavior. This has been used by ChiselTest users, we just never fully implemented it in firtool, nor for printf-encoded asserts, but we should.
Yes, it's MUCH better now! Smaller FIRRTL too (compared to printf encoding). Going to sit on this for at least a bit though to test things more thoroughly re:heavy intrinsic (expression) usage, let's ensure this sticks best way can this time 😉 . |
Restore #3912 and #3825.
Use intrinsic expression instead of intrinsic module.
This reverts commit 347d82b.
Contributor Checklist
docs/src?Type of Improvement
Desired Merge Strategy
Release Notes
Support
circt_chisel_ifelsefatalintrinsics and use it for chisel3.assert emission.Reviewer Checklist (only modified by reviewer)
3.6.x,5.x, or6.xdepending on impact, API modification or big change:7.0)?Enable auto-merge (squash), clean up the commit message, and label withPlease Merge.Create a merge commit.