-
Notifications
You must be signed in to change notification settings - Fork 421
[Arc] Complete inout support for arcilator path (Fixes #9574) #9597
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
base: main
Are you sure you want to change the base?
Conversation
fabianschuiki
left a comment
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! Thanks for adding an explicit error here 👍
|
Hi @fabianschuiki @maerhart , I rewrite this PR. Now, inout port have been supported. Please review. |
fabianschuiki
left a comment
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.
Hey @m2kar! I'm wondering how we should deal with inout ports in general. I don't think we can just lower them to llhd.sig and drive them. At the Verilog level, inout ports are nets that perform resolution among multiple drivers. We don't have such a concept yet in LLHD. The signals are more like variables that support delayed assignment and observation. We probably need something like an !llhd.net type that represents a reference to a multi-driver network. And we probably need ops that read the resolved value off of that net (maybe %0 = llhd.probe_net %n), and also to attach a driver, with potential drive condition, to that net (maybe llhd.drive_net %n, %0, %enable). That would separate the whole event queue and delayed assignment mechanism of llhd.sig from the multi-driver network and drive conflict resolution. WDYT?
fabianschuiki
left a comment
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.
(We need a multi-driver net representation in LLHD.)
[Arc] Complete inout support for arcilator path (Fixes #9574)
Summary
This PR completes inout support for the
circt-verilog --ir-hw | arcilatorflow.This specifically fixes issue #9574, where
circt-verilog --ir-hw <inout-module>.sv | arcilatorcould crash while handling inout reads.Previously,
llhd.prb/llhd.drvon inout block arguments could fail during ConvertToArcs legalization, or crash inLowerStatewhen probe reads were requested in non-Newphases.Now both SV-style and LLHD-style inout accesses are handled through Arc state lowering, and the end-to-end flow runs successfully.
Changes
ConvertToArcs updates
llhd.probe/llhd.driveon block-argument signals as arc breakers, so they are not incorrectly absorbed into extracted arcs.llhd::ProbeOp/llhd::DriveOpdynamically legal in conversion when their signal is a block argument.LowerState Pass Updates
llhd::SignalOp,llhd::ProbeOp, andllhd::DriveOpon local LLHD signals used by inout flows.llhd.sigvalues are allocated as Arc state and lowered througharc.state_read/arc.state_write, preventing Arc legalization failures.llhd::ProbeOplowering keeps phase-correct reads (module.getBuilder(phase)) so non-Newphase uses still work.Test updates
test/circt-verilog/arcilator-inout.sv: keeps the read-path regression for inout in the arcilator import flow.test/circt-verilog/inout-write-ir.sv: new regression that checks inout write semantics are materialized asllhd.drvin emitted HW/LLHD IR.test/Dialect/Arc/lower-state-errors.mlir: outdated "inout not supported" error case removed since this PR adds inout lowering support.Example
Input:
Output after
--arc-lower-state:Test Plan and Results
Targeted regression tests
Result:
PASS: CIRCT :: circt-verilog/arcilator-inout.svPASS: CIRCT :: circt-verilog/inout-write-ir.svPASS: CIRCT :: Dialect/Arc/lower-state-errors.mlirEnd-to-end run (SV -> circt-verilog -> arcilator -> C++ testbench)
E2E test module (
circt-b6/e2e/inout_e2e.sv):E2E testbench (
circt-b6/e2e/tb_inout_e2e.cpp):Coverage in this E2E:
cviaassign c = c_drv, and IR includesllhd.drv %c, ....cintoq_combatclk=0eval.cintoq_ffatposedge clkeval.Output:
Limitations
Z) resolution is still out of scope.Fixes #9574