[FIRRTL] Add lower-to-core lowering mode#10162
Conversation
8d65cae to
0bc4d0d
Compare
| } | ||
|
|
||
| FailureOr<Value> | ||
| FIRRTLLowering::lowerSimFormatString(StringRef originalFormatString, |
There was a problem hiding this comment.
I'm happy to drop this function as I realized this is subsumed by #10153.
There was a problem hiding this comment.
I agree there is overlap with the helper in #10153.
Given that #10153 currently depends on the larger #10146 stack and is harder to disentangle, I think keeping the simpler implementation local to #10162 is reasonable for now, especially since --lower-to-core is meant to be a pragmatic transitional path.
|
Thanks, this makes a lot of sense to me. |
dobios
left a comment
There was a problem hiding this comment.
Looks great! Just a little question about your choice of always lowering to a clockedassertlikes even when the clock is frivolous
| auto edge = firrtlToVerifClockEdge(opEventControl); | ||
| auto opName = op->getName().stripDialect(); | ||
| if (opName == "assert") { | ||
| verif::ClockedAssertOp::create(builder, predicate, edge, clock, enable, |
There was a problem hiding this comment.
Are clocked assertions always the best way to go? I'm not quite sure they have full support in all of the backends yet?
There was a problem hiding this comment.
Wouldn't it be easier to just emit a regular assertion if the predicate is an i1. That would at least make it immediately usable for most cases.
There was a problem hiding this comment.
Or is there already a canonicalization that handles that?
There was a problem hiding this comment.
I think FIRRTL only has clocked assert. I'm not familiar with this area, what do you think the most reasonable lowering here? (or I can keep this blank so that you can take over).
There was a problem hiding this comment.
hmm ok i guess this should be fine for now, I think it might be a good idea to push all of the backends to support this anyways
dobios
left a comment
There was a problem hiding this comment.
I think this is a good start, we might want to revisit some of this later if it starts conflicting with the verification backends too much.
Ideally those can simply be ignored in the verification backends, but it would be nice if there was an option to simply not emit them.
This will at least make going from chisel directly to the verification backends much cleaner, thanks!
I prefer the idea of having an option define which type of lowering we want rather than just emitting both in separate regions as the latter will require some annoying edge case handling downstream which i don't think should be necessary. |
fabianschuiki
left a comment
There was a problem hiding this comment.
This is an excellent idea! Having two lowering modes would give us a way to switch between status quo and the where we want to go, and then we can close the gap iteratively. LGTM!
This PR introduces an option (
--lower-to-core) to force lowering to pure core dialects (generally verif/sim) by ignoring legacy lowering.I think the unfortunate fact is that it's unrealistic to emit core dialects from LowerToHW without contaminating them with a bunch of ugly macro lowerings (e.g.,
STOP_COND, PRINTF_COND, PRINTF_COND_FRAGMENT, ASSERT_VERBOSE_COND, and __circt_lib_logging::FileDescriptor::get). These are highly specific to Chisel (or internal SiFive use cases), and this technical debt should not block circt-bmc or arcilator workflows.The new option allows us to incrementally replace each lowering where possible. For instance, maybe printf could be handled if we had a way to replace STOP_COND with an external function call defined by the FIRRTL ABI, for which firtool could provide an interpretable default.
While I hope --lower-to-core eventually becomes the default, I'm a bit pessimistic about that transition.
As we discussed in ODM we might want to put core dialect representation to a different region, but I feel just providing option feels very pragmatic (and changes needed for LowerToHW look more manageable than what I originally imagined).
Let me know what you think! @fzi-hielscher @dobios @nanjo712
AI-assisted-by: GPT-5.4