Skip to content

[FIRRTLToHW] Lower firrtl.attach to new hw.attach and migrate SV-specific lowering logic to HWToSV.#10138

Open
nanjo712 wants to merge 1 commit intollvm:mainfrom
nanjo712:feat/hw-attach-op
Open

[FIRRTLToHW] Lower firrtl.attach to new hw.attach and migrate SV-specific lowering logic to HWToSV.#10138
nanjo712 wants to merge 1 commit intollvm:mainfrom
nanjo712:feat/hw-attach-op

Conversation

@nanjo712
Copy link
Copy Markdown
Contributor

@nanjo712 nanjo712 commented Apr 6, 2026

This PR is part of #10131 .

It focuses on hw.attach and the migration of firrtl.attach lowering logic.

This is a preliminary plan I've proposed based on the current situation. Please point out any errors.

Comment thread lib/Conversion/HWToSV/HWToSV.cpp Outdated
for (auto sym : topModule.getOps<sv::MacroDeclOp>())
symbols.insert(sym.getName());
if (!symbols.count("SYNTHESIS"))
sv::MacroDeclOp::create(rewriter, loc, "SYNTHESIS");
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is illegal to mutate top-level block from hw::HWmoduleOp pass

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for pointing this out. I realize that modifying the top-level block from a module-level pass is incorrect.

Given that macro emission and #ifdef consolidation are still centralized in LowerToHW, I plan to keep the sv.macro.decl logic there for now. My goal is to use this as a stopgap until we can implement a proper global legalization pass for all HW dialect globals.

Is this acceptable as a temporary solution, or do you have other suggestions for managing these dependencies during the migration?

@nanjo712 nanjo712 force-pushed the feat/hw-attach-op branch from a7a6402 to 3c98865 Compare April 7, 2026 05:43
@nanjo712 nanjo712 force-pushed the feat/hw-attach-op branch from 3c98865 to 693b5b9 Compare April 7, 2026 05:49
@nanjo712
Copy link
Copy Markdown
Contributor Author

nanjo712 commented Apr 7, 2026

Currently, SV macro declarations are handled uniformly by LowerToHW. Forcibly removing the related logic before migrating all operations that depend on SV macros does force me to operate on top-level blocks at the HWModuleOp level, which is a mistake.

I believe we should temporarily leave macro declarations in the LowerToHW phase, assuming the downstream HWToSV phase already has the declarations (generated by the upstream LowerToHW), as a stopgap measure. We should consider migrating macro processing after all SV operations have been migrated.

Looking forward to your feedback.

@nanjo712 nanjo712 requested a review from uenoku April 7, 2026 06:56
Copy link
Copy Markdown
Member

@uenoku uenoku left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still not convinced that it's good idea to introduce hw.attach TBH. Attach feels very biased to Chisel's construct for simply connecting external module ports one to another. For me sim.alias? might be a slightly better naming.

Do you mind if you could share the use case of this, and plan for lowering it in arcilator?

@nanjo712
Copy link
Copy Markdown
Contributor Author

nanjo712 commented Apr 8, 2026

For me sim.alias? might be a slightly better naming.

I've looked into the existing lowering logic in LowerToHW. Currently:

  • It emits an all-pairs assign complex for synthesis (as a workaround for tools that don't support alias).

  • It emits a verbatim error for Verilator.

  • It emits sv.alias only as a fallback.

Since the "all-pairs assign" case is clearly a hardware synthesis requirement, calling it sim.alias would be misleading as it's not restricted to simulation.

@nanjo712
Copy link
Copy Markdown
Contributor Author

nanjo712 commented Apr 8, 2026

Attach feels very biased to Chisel's construct for simply connecting external module ports one to another.

import chisel3.experimental.{attach, Analog}

class AttachExample extends RawModule {
    val a = IO(Analog(8.W))
    val b = IO(Analog(8.W))
    val c = IO(Analog(8.W))

    attach(a, b, c)
}

Code like this will create a firrtl.attach, and when emitted as HWDialect. It will be:

module {
  sv.macro.decl @SYNTHESIS
  sv.macro.decl @VERILATOR
  hw.module @AttachExample(inout %a : i8, inout %b : i8, inout %c : i8) {
    %0 = sv.read_inout %c : !hw.inout<i8>
    %1 = sv.read_inout %b : !hw.inout<i8>
    %2 = sv.read_inout %a : !hw.inout<i8>
    sv.ifdef  @SYNTHESIS {
      sv.assign %a, %1 : i8
      sv.assign %a, %0 : i8
      sv.assign %b, %2 : i8
      sv.assign %b, %0 : i8
      sv.assign %c, %2 : i8
      sv.assign %c, %1 : i8
    } else {
      sv.ifdef  @VERILATOR {
        sv.verbatim "`error \22Verilator does not support alias and thus cannot arbitrarily connect bidirectional wires and ports\22"
      } else {
        sv.alias %a, %b, %c : !hw.inout<i8>, !hw.inout<i8>, !hw.inout<i8>
      }
    }
    hw.output
  }
  om.class @AttachExample_Class(%basepath: !om.basepath) {
    om.class.fields 
  }
}

If we directly downgrade firrtl.attach to a bunch of sv.ifdef and sv.assign, it's nearly impossible for Arcilator to recover its simple "aliasing" intent. By preserving it as hw.attach, we can provide the backend with a clear IR.

plan for lowering it in arcilator

Because Arcilator flattens the hierarchy, it's well-suited for performing variable merging. It can choose a representative SSA value for the entire alias set and replace all other references.

Even in the worst case, the backend could downgrade it to a C++ reference or a simple pointer, but this choice should be determined by the backend itself, not enforced by the LowerToHW procedure.

However, this is just my initial thought and not a complete plan. Please point out any errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants