[ImportVerilog] Preserve declaration order across regular and interface-modport ports#10468
Open
micprog wants to merge 2 commits into
Open
[ImportVerilog] Preserve declaration order across regular and interface-modport ports#10468micprog wants to merge 2 commits into
micprog wants to merge 2 commits into
Conversation
micprog
force-pushed
the
micprog/modport_order
branch
from
da1902c to
ae1a819
Compare
fabianschuiki
approved these changes
May 20, 2026
Contributor
fabianschuiki
left a comment
fabianschuiki
left a comment
There was a problem hiding this comment.
Really nice, thanks a lot for fixing this! 🥳 LGTM! Just a few nits.
Comment on lines
+595
to
+601
| if (port.ast.direction == ArgumentDirection::Out) { | ||
| assert(port.outputIdx && "output port missing outputIdx"); | ||
| outputValues[*port.outputIdx] = value; | ||
| } else { | ||
| assert(port.inputIdx && "input port missing inputIdx"); | ||
| inputValues[*port.inputIdx] = value; | ||
| } |
Contributor
There was a problem hiding this comment.
I think you can drop the asserts here, since std::optional will already assert that the value exists when you * dereference it:
Suggested change
| if (port.ast.direction == ArgumentDirection::Out) { | |
| assert(port.outputIdx && "output port missing outputIdx"); | |
| outputValues[*port.outputIdx] = value; | |
| } else { | |
| assert(port.inputIdx && "input port missing inputIdx"); | |
| inputValues[*port.inputIdx] = value; | |
| } | |
| if (port.ast.direction == ArgumentDirection::Out) | |
| outputValues[*port.outputIdx] = value; | |
| else | |
| inputValues[*port.inputIdx] = value; |
Comment on lines
+634
to
+635
| assert(fp.outputIdx && "output iface port missing outputIdx"); | ||
| outputValues[*fp.outputIdx] = val; |
Contributor
There was a problem hiding this comment.
Same here:
Suggested change
| assert(fp.outputIdx && "output iface port missing outputIdx"); | |
| outputValues[*fp.outputIdx] = val; | |
| outputValues[*fp.outputIdx] = val; |
Comment on lines
+641
to
+642
| assert(fp.inputIdx && "input iface port missing inputIdx"); | ||
| inputValues[*fp.inputIdx] = val; |
Contributor
There was a problem hiding this comment.
Same here:
Suggested change
| assert(fp.inputIdx && "input iface port missing inputIdx"); | |
| inputValues[*fp.inputIdx] = val; | |
| inputValues[*fp.inputIdx] = val; |
Comment on lines
+1375
to
+1376
| assert(port.outputIdx && "output port missing outputIdx"); | ||
| outputs[*port.outputIdx] = value; |
Contributor
There was a problem hiding this comment.
Suggested change
| assert(port.outputIdx && "output port missing outputIdx"); | |
| outputs[*port.outputIdx] = value; | |
| outputs[*port.outputIdx] = value; |
| if (!ref) | ||
| continue; | ||
| outputs.push_back(moore::ReadOp::create(builder, fp.loc, ref).getResult()); | ||
| assert(fp.outputIdx && "output iface port missing outputIdx"); |
Contributor
There was a problem hiding this comment.
Suggested change
| assert(fp.outputIdx && "output iface port missing outputIdx"); |
|
Results of circt-tests run for 1bf5389 compared to results for 95535c5: sv-testsChanges in emitted diagnostics:
Introduced 1 segfault:
|
micprog
force-pushed
the
micprog/modport_order
branch
from
1bf5389 to
5ff01db
Compare
|
Results of circt-tests run for 5ff01db compared to results for 95535c5: sv-testsChanges in emitted diagnostics:
Introduced 1 segfault:
|
Contributor
|
The changes in error messages in sv-tests in circt-tests is interesting. I guess some of this might be related to failing tests now running further and hitting other issues, which is great. But some new errors are related to hierarchical paths, which could be an issue with the port order potentially? If you're feeling adventurous @micprog, circt-tests is fairly straightforward to clone and run. That'll allow you to look at that segfault in |
Contributor
|
Actually now that I think about this some more: this might be due to a bump in sv-tests. Try rebasing your PR onto current main. (I've noticed that we've picked up one of the segfaults you're seeing since the last sv-tests bump, so this likely has nothing to do with your PR!) |
micprog
force-pushed
the
micprog/modport_order
branch
from
5ff01db to
0302638
Compare
|
Results of circt-tests run for 0302638 compared to results for 95535c5: sv-testsChanges in emitted diagnostics:
Introduced 1 segfault:
|
micprog
force-pushed
the
micprog/modport_order
branch
from
0302638 to
32cfab2
Compare
|
Results of circt-tests run for 32cfab2 compared to results for 95535c5: sv-testsChanges in emitted diagnostics:
Introduced 1 segfault:
|
Contributor
Author
|
Thanks for the feedback @fabianschuiki! I removed the asserts and rebased the branch, hoping to clean up the bot's report, but it looks like there are still other issues. The bot originally reported 0 change (+1/-1), I don't see how these changes could affect this large number of items. The segfault is reproducible on the current main, I'm fairly certain it's an issue of the base this PR is comparing to (Could the last run on main that the bot is comparing to still reference an older sv-tests version?). I can dig into the issue (AI-assisted), but I don't think it makes sense to address in this PR as it is a separate issue. |
micprog
force-pushed
the
micprog/modport_order
branch
from
32cfab2 to
3769996
Compare
|
Results of circt-tests run for 3769996 compared to results for 95535c5: sv-testsChanges in emitted diagnostics:
Introduced 1 segfault:
|
…ce-modport ports When a module's port list interleaves an interface-modport port with regular outputs, the instance-port plumbing produced `moore.instance` / `moore.output` verifier failures of the form `operand N (...) does not match input/output type (...) of module @M`. `convertModuleHeader` walked the AST port list in order and pushed each port (regular or expanded interface-modport) to `modulePorts` interleaved by declaration order. But `convertModuleBody` built the `moore.output` operand vector by walking `lowering.ports` first (regular outputs only) and then `lowering.ifacePorts` (interface outputs only), and the visit-instance code did the same for the `moore.instance` operand and result vectors. Whenever the declaration order had an interface-modport output before a regular output, the operand vector and the module signature were permutations of each other and verification fired. Fix: record an output slot index (`outputIdx`) on `PortLowering` and `FlattenedIfacePort` at header time, and a matching input slot index (`inputIdx`). Track `numExplicitOutputs` / `numExplicitInputs` on `ModuleLowering`. Build `outputs` / `inputValues` / `outputValues` as fixed-size vectors and place values by slot index, both at the module's terminator and at the instance call site. Hierarchical-name ports keep their existing append-after-explicit-ports treatment. Regression test: `interface-port-order.sv` exercises a declaration order with an interface-modport output (l32) before a regular output (l64). The widths differ so any swap trips the `moore.output` terminator's verifier (the terminator is rigid, with no `materializeConversion` between operands and declared port types). Assisted-by: Claude Code:claude-opus-4-7
micprog
force-pushed
the
micprog/modport_order
branch
from
3769996 to
19b380f
Compare
Contributor
Author
|
@fabianschuiki I think we identified the issue - circt-tests compares to the main branch in the fork's repository, not the target (llvm/circt) repository. My main branch was out of date, causing circt-bot to use an outdated baseline. I opened circt/circt-tests#21 to address this. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When a module's port list interleaves an interface-modport port with regular outputs, the instance-port plumbing produced
moore.instance/moore.outputverifier failures of the formoperand N (...) does not match input/output type (...) of module @M.convertModuleHeaderwalked the AST port list in order and pushed each port (regular or expanded interface-modport) tomodulePortsinterleaved by declaration order. ButconvertModuleBodybuilt themoore.outputoperand vector by walkinglowering.portsfirst (regular outputs only) and thenlowering.ifacePorts(interface outputs only), and the visit-instance code did the same for themoore.instanceoperand and result vectors. Whenever the declaration order had an interface-modport output before a regular output, the operand vector and the module signature were permutations of each other and verification fired.Fix: record an output slot index (
outputIdx) onPortLoweringandFlattenedIfacePortat header time, and a matching input slot index (inputIdx). TracknumExplicitOutputs/numExplicitInputsonModuleLowering. Buildoutputs/inputValues/outputValuesas fixed-size vectors and place values by slot index, both at the module's terminator and at the instance call site. Hierarchical-name ports keep their existing append-after-explicit-ports treatment.Regression test:
interface-port-order.svexercises a declaration order with an interface-modport output (l32) before a regular output (l64). The widths differ so any swap trips themoore.outputterminator's verifier (the terminator is rigid, with nomaterializeConversionbetween operands and declared port types).Assisted-by: Claude Code:claude-opus-4-7