[FIRRTLToHW] Remove the SYNTHESIS guard in printf/flush lowering path#10526
[FIRRTLToHW] Remove the SYNTHESIS guard in printf/flush lowering path#10526nanjo712 wants to merge 7 commits into
Conversation
|
Hi @seldridge, since you mentioned that you could try running this through internal builds/flows to see whether anything breaks, would you mind taking a look when you get a chance? I’d like to confirm whether removing this guard causes any downstream impact. |
nanjo712
force-pushed
the
feat/remove-sysnthesis-guard
branch
from
92397fb to
19dc84e
Compare
seldridge
left a comment
seldridge
left a comment
There was a problem hiding this comment.
Awesome, thanks for doing this.
@nanjo712: yes, I can take a look at this. I'm currently working on a 1.148.0 release and will hold off on testing this. We've got a number of outstanding bugs and one bug fix that needs to get released soon. Once I have something clean on our internal builds, I can take a look at this.
|
Thanks for taking a look! |
|
This passed some light internal testing. I also inspected the Verilog for one sample and the result looks fine. We have everything already guarded by layers, so there is no real need for the double-guarding. I'm good to proceed with this. |
seldridge
left a comment
seldridge
left a comment
There was a problem hiding this comment.
Generally LGTM. Internal testing looked fine. (I'm not saying I won't have to revert it later when I realize this is problematic, though!)
Question about how this is finding the macro.
nanjo712
force-pushed
the
feat/remove-sysnthesis-guard
branch
from
933001f to
4367640
Compare
|
Well, I don't really understand what's happening in CI process. It failed in the stage It looks like a environment problem. |
|
The issue disappeared after I re-triggered the CI. I tried moving the previously mentioned helper to a more reusable location and adjusted my previous logic. It should now be ready for further review. 🚀 |
seldridge
left a comment
seldridge
left a comment
There was a problem hiding this comment.
This is looking great. One comment about the function signature and what is passed around.
| StringAttr resolveMacroSymName(Operation *symbolTableOp, StringRef verilogName, | ||
| bool &created); |
There was a problem hiding this comment.
Nit: naming this getOrCreate may be clearer:
| StringAttr resolveMacroSymName(Operation *symbolTableOp, StringRef verilogName, | |
| bool &created); | |
| StringAttr getOrcreate(Operation *symbolTableOp, StringRef verilogName, | |
| bool &created); |
There was a problem hiding this comment.
This function doesn't actually create anything; it just checks if a symbol already exists or generates a usable symbol name. Perhaps it could be replaced with lookupOrGenerateMacroSymName?
|
@nanjo712 wrote:
I've seen this happen before. It appears to be intermittent. LLMs seem to think that PowerShell Gallery is just flaky. 🫠 Maybe we could change the script to do a few retries before it finally fails. |
| Namespace ns; | ||
| auto symbolNameId = | ||
| StringAttr::get(symbolTableBlock->getParentOp()->getContext(), | ||
| SymbolTable::getSymbolAttrName()); | ||
| for (auto &op : *symbolTableBlock) { | ||
| if (auto symbol = op.getAttrOfType<StringAttr>(symbolNameId)) | ||
| ns.add(symbol.getValue()); | ||
| } | ||
|
|
||
| created = true; | ||
| return StringAttr::get(symbolTableBlock->getParentOp()->getContext(), | ||
| ns.newName(verilogName)); | ||
| } |
There was a problem hiding this comment.
I understand it existed before, but could you consider using SymbolTable::insert instead (as SymbolTable is MLIR proper)?
There was a problem hiding this comment.
I may have misunderstood. I think SymbolTable::insert is not quite appropriate here because this helper simply selects a symbol name and intentionally postpones the creation of the decl opeartion.
| auto fragment = emit::FragmentOp::create(builder, fragmentSymName, [&] { | ||
| emitGuard("SYNTHESIS", [&]() { | ||
| emitGuard(kFileDescriptorMacroName, [&]() { | ||
| emitGuard(synthesisSymName, [&]() { |
There was a problem hiding this comment.
I would be more cautious about removing this. We could simply remove the synthesis guard for print operations, mainly because Chisel places these operations in a separate layer, which are then output to separate files. These separate files themselves have macro guard, and also it's easy to disable it by not including these separate files in the synthesis.
As far as I know, there doesn't seem to be a similar mechanism for this part; it outputs separately to each file used, unlike print operations which are separated from the design code.
We might be able to improve it further, but I think this might require another evaluation.
3 participants
As discussed in #10489, for operations like printing, SYNTHESIS guard may no longer be necessary because Chisel will send these operations to the verification layer.