-
Notifications
You must be signed in to change notification settings - Fork 278
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FIRRTL] Remove support for printf-encoded verification, tests. #7010
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! What happens if someone feeds a FIR file with some older FIR version? Do they get the deprecation error added by that other PR?
No, they get what their FIRRTL spec says they get, a printf. |
However, breaking compatibility with what Chisel produced in the past (ancient or recent) is not something we have a story for, and this is core enough that dropping this means breaking compatibility with quite a long history of FIRRTL. Bummer. I'm not sure maintaining the ability to parse these things /without/ the ability to generate the corresponding IR is worth bothering with, so I think this means this lives on indefinitely. |
Hmmm good point, it's not actually in the spec. It just happened to work, like an intrinsic without any clear definition of what it does. It might be worthwhile to just rip off the bandaid, and drop support for this from firtool. Up-to-date versions of Chisel will keep working, and everything else breaks. Or we could allocate some grace period while this still works with a warning, and then pull the plug in say 3 months? Would be a shame to not delete old stale code. |
if (fmt.contains("[verif-library-assert]")) | ||
flavor = VerifFlavor::VerifLibAssert; | ||
else if (fmt.contains("[verif-library-assume]")) | ||
flavor = VerifFlavor::VerifLibAssume; | ||
else if (fmt.contains("[verif-library-cover]")) | ||
flavor = VerifFlavor::VerifLibCover; | ||
else if (fmt.consume_front("assert:")) | ||
flavor = VerifFlavor::Assert; | ||
else if (fmt.consume_front("assume:")) | ||
flavor = VerifFlavor::Assume; | ||
else if (fmt.consume_front("cover:")) | ||
flavor = VerifFlavor::Cover; | ||
else if (fmt.consume_front("assertNotX:")) | ||
flavor = VerifFlavor::AssertNotX; | ||
else if (fmt.starts_with("Assertion failed")) | ||
flavor = VerifFlavor::ChiselAssert; | ||
else | ||
return false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure it's worth doing but how about extracting this part and emitting warnings for PrintOp with such format string? Though there might be false-alarm but I feel it's better than silently dropping assertions. It would be not that bad to inspect format string at parsing.
Add FIR -> SV test for "unclocked_assert" since we care about ensuring it has a specific shape re:output. Test extracted from llvm#7010.
Add FIR -> SV test for "unclocked_assert" since we care about ensuring it has a specific shape re:output. Test extracted from #7010.
Printf-encoded verification ops were always a SiFive-internal hack. If this got hop-on external users, they knew what they were getting into (also a user sufficiently motivated to reverse engineer these printf-encoded assertions is likely still motivated to use the documented replacements). Drop 'em. I see no risk here. |
Note that chisel assert has been also encoded as printf so it could be user-facing for external users. |
bee8928
to
9cdb5d7
Compare
Created #7030 to run with the idea of removing support but retaining classification/recognition to diagnose unsupported use of these printf's, FWIW. |
9cdb5d7
to
0e2cd31
Compare
PR updated to remove the rest not already removed: tests and the detection + error introduced with #7030 . Not rejecting Chisel-produced variants has at least some related/reasonable behavior (printf, stop, assert -- they'll still generally do the right thing), but the other flavors produced by rocketchip or similar (e.g., "this is really a cover" in printf means assert is a cover) won't work and designs built on this will be broken/wrong. |
re-open in the future when ready to drop this completely. |
Add small test for load-bearing "UNR" behavior especially interaction with
--add-companion-assume
.