Plumb dbg.variable anchors into SMT symbol names#10237
Plumb dbg.variable anchors into SMT symbol names#10237
Conversation
|
@TaoBi22 would love your thoughts on this implementation! |
TaoBi22
left a comment
There was a problem hiding this comment.
Thanks for looking into this @5iri! A couple of preliminary comments below - I think the main question is whether it makes sense to do this in ExternalizeRegisters or in a separate pass (more discussion of that in the comment directly)
| // CHECK-LABEL: func.func @bmc_debug_anchors_to_names() -> i1 { | ||
| // CHECK: smt.declare_fun "port_data" : !smt.bv<8> | ||
| // CHECK: smt.declare_fun "state_data" : !smt.bv<8> | ||
| // CHECK: smt.declare_fun "port_data" : !smt.bv<8> | ||
| // CHECK-NOT: dbg.variable |
There was a problem hiding this comment.
It would be nice to check a bit more of the IR here to make sure that the right names are being given to the right functions
| void ExternalizeRegistersPass::materializeDebugVariables(HWModuleOp module) { | ||
| auto *body = module.getBodyBlock(); | ||
| DenseSet<Value> trackedValues; | ||
| for (auto varOp : body->getOps<debug::VariableOp>()) | ||
| trackedValues.insert(varOp.getValue()); | ||
|
|
||
| DenseSet<StringAttr> outputPortNames; | ||
| for (auto &port : module.getPortList()) | ||
| if (port.isOutput()) | ||
| outputPortNames.insert(port.name); | ||
|
|
||
| OpBuilder builder = OpBuilder::atBlockBegin(body); | ||
| StringRef stateSuffix = "_state"; | ||
| for (auto &port : module.getPortList()) { | ||
| if (port.isOutput()) | ||
| continue; | ||
| if (isa<seq::ClockType>(port.type)) | ||
| continue; | ||
| if (!port.name || port.name.getValue().empty()) | ||
| continue; | ||
|
|
||
| auto value = body->getArgument(port.argNum); | ||
| if (!trackedValues.insert(value).second) | ||
| continue; | ||
|
|
||
| StringAttr debugName = port.name; | ||
| auto portName = port.name.getValue(); | ||
| if (portName.ends_with(stateSuffix)) { | ||
| auto baseName = portName.drop_back(stateSuffix.size()); | ||
| auto nextNameAttr = | ||
| builder.getStringAttr((Twine(baseName) + "_next").str()); | ||
| if (outputPortNames.contains(nextNameAttr)) | ||
| debugName = builder.getStringAttr(baseName); | ||
| } | ||
|
|
||
| debug::VariableOp::create(builder, value.getLoc(), debugName, value, | ||
| /*scope=*/Value{}); |
There was a problem hiding this comment.
What's the reasoning behind doing this in ExternalizeRegisters? It seems quite distinct from the existing function of the pass, it might be sensible to put this in a separate pass (which it would be nice to split into a different PR from the VerifToSMT changes)
There was a problem hiding this comment.
Yes! I think yes it is sensible to put this in a seperate pass, like materialize-debug-anchors. I was more focused on getting to produce the correct dbg.variable anchors without extra plumbing.
That said, I am happy to split this into a dedicated pass and then a follow up PR where VerifToSMT consumes those anchors.
I can rework this so ExternalizeRegisters stays focused and the pipeline composes the two passes explicitly.
| for (auto varOp : debugOpsToErase) | ||
| rewriter.eraseOp(varOp); |
There was a problem hiding this comment.
nit: if you use llvm::make_early_inc_range you should be able to do this in the loop directly
This PR improves BMC traceability by propagating
dbg.variableanchors into VerifToSMT symbol names.dbg.variableanchors inexternalize-registersfor non-clock module inputs.For example,
can produce
Key part of the output:
%port_data = smt.declare_fun "port_data" : !smt.bv<8>%state_data = smt.declare_fun "state_data" : !smt.bv<8>So the debug anchors are being consumed correctly, and SMT symbols use meaningful names (port_data,state_data) instead of only input_N/reg_N.