[FIRRTL] Export named references as macros to Verilog files #5180

trilorez · 2023-05-12T00:09:38Z

This implements the proposed lowering of ref ports in the firrtl abi spec.

During the LowerXMR pass, for each public output ref port, an hw.exportableRef op is created to track the ref. This op is a symbolic namepath for the ref. During the ExportVerilog pass, sv.macro.decl and sv.macro.def ops are created with the fully resolved namepath based on the legalized verilog names.

Although the semantics are fairly firrtl specific, the exportable-ref op is part of the hw dialect since it needs to hang around until ExportVerilog.

seldridge · 2023-05-12T04:57:43Z

include/circt/Dialect/HW/HWStructure.td

+                       StrAttr:$refName, 
+                       InnerRefArrayAttr:$namepath);


Can this instead use a symbol referring to a HierPathOp? Or: is there a reason to not use one?

HierPathOp doesn't work as is because it enforces the namepath to be at least 2 elements long. The path for a ref port can be one element long if it's a direct reference to a wire instead of through an instance. I'm not sure what the consequences of lifting the path length restriction are.

HierPathOp also allows for the path to end with a FlatSymbolRef to a module which doesn't make sense for this use case but that's not a big deal.

I can see if allowing for one element works.

What about the approach of XMRRefOp? This accepts either a symbol pointing at a HierPathOp or an InnerRefAttr? (It does this for exactly the reason of the former requiring namepath size that is >= 2.)

Thinking about your second point: we can lock this down with a verifier. However, I'd have to think if there is a fundamental reason why we don't want that ability in the exported refs. E.g., to get a handle to an instance. (I admit we don't use this now and the FIRRTL ABI doesn't mention it.)

Good point, I'll try the XMRRefOp approach

That HierPathOp allows ending with a FlatSymbolRef doesn't really matter. We need to lower to a correct construct, not a construct that is only as wide as we need (for the same reason we don't have add and addWithConstant as separate).

It does this for exactly the reason of the former requiring namepath size that is >= 2

We have examples in the test which only have one element: hw.hierpath private @ref2 [@wait_order::@baz]

I'm confused about that, because sizes less than 2 are disallowed here. Is verifyInnerRefs not run when not inside a firrtl.circuit block?

Yes, all of inner symbol verification + datastructures (+InnerSymDCE) do not work for HW, despite living there, which yes includes nothing driving verifyInnerRefs -- which is driven as verification of op defining "InnerRefNamespace" (firrtl.circuit), which contains InnerSymbolTables named by Symbols, and inner symbols point to things within those (so inner ref pair of symbol/innersymbol resolves that way). See #4453 and the graph of linked 10000 issues (or search for it). This should probably be reworked if cannot be adapted.

seldridge · 2023-05-12T05:02:09Z

lib/Dialect/FIRRTL/Transforms/LowerXMR.cpp

+    if (!topModule)
+      return success();


I had to double check why this was necessary, but this is catching the case of an external module being the top module.

Is this even allowed? Verify seems to say allowed. Spec implies there has to be a real module, but is ambiguous.

Both SFC and MFC accept the top module being an external module. We could disallow it without consequence I think.

Right now you can put an external module as the top module in a circuit and use that as a separately compiled unit. However, the compilation of that is basically a no-op. The client using this also has to have an external module pointing at this circuit containing another extmodule. I don't see why we need to enable such redirection (duplication?).

seldridge · 2023-05-12T05:11:08Z

lib/Dialect/FIRRTL/Transforms/LowerXMR.cpp

+      if (!refType || isZeroWidth(refType.getType()) ||
+          topModule.getPortDirection(portIndex) != Direction::Out)
+        continue;


We may have to think about how to handle the zero width case. We may need to generate the macro as '0. TBD.

commented out macro? What would it point to? Since 0-width elements are generally removed, I don't see why a ref to a zero width element wouldn't also.

seldridge · 2023-05-12T05:12:40Z

lib/Dialect/HW/HWOps.cpp

+        ArrayAttr::get(odsBuilder.getContext(), namePath));
+}
+
+LogicalResult ExportableRefOp::verifyInnerRefs(hw::InnerRefNamespace &ns) {


Using a HierPathOp symbol would avoid the duplication of this checking here.

seldridge · 2023-05-12T05:13:31Z

lib/Dialect/HW/HWOps.cpp

+  return success();
+}
+
+void ExportableRefOp::print(OpAsmPrinter &p) {


Are custom printer/parsers necessary or can we get by with the default ones if using a HierPathOp?

seldridge · 2023-05-12T05:14:07Z

test/firtool/export-ref.fir

+; CHECK:      `define ref_Top_Top_direct_probe direct
+; CHECK-NEXT: `define ref_Top_Top_inner_probe inner.x
+; CHECK-NEXT: `define ref_Top_Top_keyword_probe always_0


This looks awesome.

uenoku · 2023-05-12T09:55:46Z

I'm wondering we can avoid introducing a new operation, instead I think we can directly use MacroDefOp at LowerXMROp by extending with symbol substitution as we are doing for VerbatimOp so that we can substitute legalized module names, e.g. sv.macro.def @ref_Top_Top_inner_probe "{{0}.{{1}}" {symbols = [@Inner, #hw.innerNameRef<@Inner::@xmr_sym>]}. WDYT?

Other comments if we will introduce a new operation:

Can we move IR mutation into PrepareForEmission? We have avoided IR mutation in ExportVerilog as much as possible and I think you had to mutate IR because legalized module names were used by macro definition.
As you mentioned, this hw.exportableRef op encodes ABI extremely specific to FIRRTL and I believe it's hard to generalize to other frontends. Therefore can we consider adding fir prefix or something to indicate that this is FIRRTL specific?

trilorez · 2023-05-12T15:36:40Z

I'm wondering we can avoid introducing a new operation, instead I think we can directly use MacroDefOp at LowerXMROp by extending with symbol substitution as we are doing for VerbatimOp so that we can substitute legalized module names, e.g. sv.macro.def @ref_Top_Top_inner_probe "{{0}.{{1}}" {symbols = [@Inner, #hw.innerNameRef<@Inner::@xmr_sym>]}. WDYT?

I think this can work, although we then lose the verification of the namepath until we try to substitute it at the end.

1. Can we move IR mutation into PrepareForEmission? We have avoided IR mutation in ExportVerilog as much as possible and I think you had to mutate IR because legalized module names were used by macro definition.

I don't think so, because legalizeGlobalNames occurs after PrepareForEmission at the beginning of exportVerilogImpl. I actually originally just had ExportableRefOp directly emit the macro text. I updated my code to use the new sv macro ops since that seems better structured and leaves a record of the macro in the final ir, but I could go back to my original approach, which doesn't mutate the ir.

2. As you mentioned, this `hw.exportableRef` op encodes ABI extremely specific to FIRRTL and I believe it's hard to generalize to other frontends. Therefore can we consider adding `fir` prefix or something to indicate that this is FIRRTL specific?

That makes sense. Or, we can add a convention attribute to the op and start with just supporting a firrtl convention, which leaves room for expanding to other conventions.

seldridge · 2023-05-12T15:38:00Z

@uenoku wrote:

I'm wondering we can avoid introducing a new operation, instead I think we can directly use MacroDefOp at LowerXMROp by extending with symbol substitution as we are doing for VerbatimOp so that we can substitute legalized module names, e.g. sv.macro.def @ref_Top_Top_inner_probe "{{0}.{{1}}" {symbols = [@Inner, #hw.innerNameRef<@Inner::@xmr_sym>]}.

This is brittle as it locks in the structure of the string encoding the path. E.g., if module @inner is ever inlined, the inliner doesn't know what the string encoding means so it doesn't know how to update it. (Noting that inlining would produce an invalid hierpath op 😬 ).

Everything is easier if we separate the hierarchical path from the string, like how XMRRefOp works. That way any hierarchical path manipulation can happen because 67ED we know how to keep hier path ops up to date.

mwachs5 · 2023-05-12T16:27:15Z

Is there a test case where the reference is to a literal value rather than a wire or reg? As I think that is allowed...?

darthscsi

We really shouldn't need new ops for this and we shouldn't be making macros in export verilog. The macro should be created at lowering from firrtl.

darthscsi · 2023-05-12T17:15:47Z

lib/Conversion/ExportVerilog/ExportVerilog.cpp

+/// Create a MacroDeclOp and MacroDefOp for the ExportableRefOp.
+/// This is called before the symbolCache has been frozen,
+/// so the MacroDefOp will not have it's format string set yet.
+static MacroDefOp createExportableRefMacro(ExportableRefOp op) {


ExportVerilog shouldn't be creating new macros, it should be emitting macros that already exist.

darthscsi · 2023-05-12T17:17:07Z

lib/Dialect/FIRRTL/Transforms/LowerXMR.cpp

+      if (!refType || isZeroWidth(refType.getType()) ||
+          topModule.getPortDirection(portIndex) != Direction::Out)
+        continue;


commented out macro? What would it point to? Since 0-width elements are generally removed, I don't see why a ref to a zero width element wouldn't also.

trilorez · 2023-05-12T17:25:24Z

We really shouldn't need new ops for this and we shouldn't be making macros in export verilog. The macro should be created at lowering from firrtl.

We can't generate the verbatim macro until names are legalized. Are you suggesting to do something similar to what @uenoku proposed with using symbol substitution?

trilorez · 2023-05-12T17:26:24Z

Is there a test case where the reference is to a literal value rather than a wire or reg? As I think that is allowed...?

Not currently, I can add one

darthscsi · 2023-05-12T17:37:49Z

We really shouldn't need new ops for this and we shouldn't be making macros in export verilog. The macro should be created at lowering from firrtl.

We can't generate the verbatim macro until names are legalized. Are you suggesting to do something similar to what @uenoku proposed with using symbol substitution?

Yes.

seldridge · 2023-05-12T18:15:26Z

Here's an example showing the zero width problem:

//> using scala "2.13.10"
//> using repository "https://s01.oss.sonatype.org/content/repositories/snapshots"
//> using lib "org.chipsalliance::chisel::6.0.0-M1+3-35860474-SNAPSHOT"
//> using plugin "org.chipsalliance:::chisel-plugin::6.0.0-M1+3-35860474-SNAPSHOT"
//> using options "-unchecked", "-deprecation", "-language:reflectiveCalls", "-feature", "-Xcheckinit", "-Ywarn-dead-code", "-Ywarn-unused", "-Ymacro-annotations"

import chisel3._
import chisel3.util.experimental.BoringUtils
import circt.stage.ChiselStage

class Bar extends Module {
  val a = WireInit(UInt(0.W), DontCare)
  dontTouch(a)
}

class Foo extends Module {
  val a = IO(Output(UInt(1.W)))

  val bar = Module(new Bar)
  a := BoringUtils.tapAndRead(bar.a)
}

object Main extends App {
  println(
    ChiselStage.emitSystemVerilog(
      new Foo,
      firtoolOpts = Array("-strip-debug-info")
    )
  )
}

This currently produces the following FIRRTL:

circuit Foo :%[[
  {
    "class":"firrtl.transforms.DontTouchAnnotation",
    "target":"~Foo|Bar>a"
  }
]]
  module Bar :
    input clock : Clock
    input reset : Reset
    output a_bore : Probe<UInt<0>> 

    wire a : UInt<0> 
    a is invalid 
    define a_bore = probe(a) 

  module Foo :
    input clock : Clock
    input reset : UInt<1>
    output a : UInt<1> 

    inst bar of Bar 
    bar.clock <= clock
    bar.reset <= reset
    a <= read(bar.a_bore)

This currently produces the following Verilog:

module Bar();
endmodule

module Foo(
  input  clock,
         reset,
  output a
);

  Bar bar ();
  assign a = 1'h0;
endmodule

The problem is that this has a legal reference to something, but that something is a zero-width thing. If Foo is compiled independently of Bar, then either Foo needs to know the width and it can substitute in a '0. Alternatively, the compilation of Bar needs to produce a define that is '0 or a define pointing at something in Bar that has a '0 value.

This is further complicated if we continue to allow unknown width output ports as the signature of Bar is insufficient to know if Foo needs to drop a '0 or not. Consider instead:

class Bar extends Module {
  val a = Wire(UInt())
  dontTouch(a)
  a := WireInit(UInt(0.W), DontCare)
}

And FIRRTL:

circuit Bar :
  module Bar :
    input clock : Clock
    input reset : Reset
    output a_bore : Probe<UInt> 

    wire a : UInt 
    wire _a_WIRE : UInt<0> 
    _a_WIRE is invalid 
    a <= _a_WIRE 
    define a_bore = probe(a)

darthscsi · 2023-05-12T18:46:43Z

For reference, here was the PR I was working on for this: https://github.com/llvm/circt/pull/5169/files

darthscsi · 2023-05-12T19:04:53Z

We don't allow uninferred widths on public modules, I don't see why we would allow probes of unknown widths on public modules. But I don't think we need to produce something for a ref to 0 as we've defined that lowering as not existing,. It may be convinient to allow it in operations at the firrtl level, but that doesn't mean those operations (on a ref to 0) will themselves lower to anything.

trilorez · 2023-05-12T19:27:52Z

I've removed the ExportableRefOp and now directly insert the sv macro ops in LowerXMR, 10000 with symbolic substitution support added for macros.

Since MacroDeclOp is required to be under mlir::ModuleOp, this means LowerXMR must be run on mlir::ModuleOp instead of CircuitOp.

darthscsi · 2023-05-12T20:03:53Z

lib/Dialect/FIRRTL/Transforms/LowerXMR.cpp

@@ -418,6 +425,62 @@ class LowerXMRPass : public LowerXMRBase<LowerXMRPass> {
    return success();
  }

+  LogicalResult handleTopModuleRefPorts() {


Should this remove the ref ports from the module?

All ref ports are already removed in the garbageCollect function that's called after this.

I suppose you could do it for the top module here, although if we expand this to support multiple "public" modules in a circuit, it wouldn't be safe to start deleting ref ports here in case one public ref port has dataflow to another one in a separate module.

we do need to do all public modules.

I don't care were they are deleted, as long as they are.

trilorez · 2023-05-12T20:30:55Z

Is there a test case where the reference is to a literal value rather than a wire or reg? As I think that is allowed...?

I added a test-case on the mlir side. I'm not sure if you can actually express that in a fir file.

darthscsi · 2023-05-12T21:01:27Z

include/circt/Dialect/SV/SVStatements.td

  }];

  let arguments = (ins FlatSymbolRefAttr:$macroName, 
-                       StrAttr:$format_string);
+                       StrAttr:$format_string,
+                       DefaultValuedAttr<NameRefArrayAttr,"{}">:$symbols);


OptionalAttr<FlatSymbolRefArrayAttr>:$symbols ?
DefaultValuedAttr<FlatSymbolRefArrayAttr,"{}">:$symbols ?

darthscsi · 2023-05-12T21:02:13Z

include/circt/Dialect/SV/SVStatements.td

+                "build(odsBuilder, odsState, macroName, format_string,"
+                "odsBuilder.getArrayAttr({}));">
+  ];
+
  let assemblyFormat = [{
    $macroName $format_string attr-dict


$macroName $format_string (`(` $symbols^ `)`)? attr-dict

They should be explicit.

darthscsi · 2023-05-12T21:03:04Z

lib/Dialect/FIRRTL/Transforms/LowerXMR.cpp

@@ -418,6 +425,62 @@ class LowerXMRPass : public LowerXMRBase<LowerXMRPass> {
    return success();
  }

+  LogicalResult handleTopModuleRefPorts() {


we do need to do all public modules.

darthscsi · 2023-05-12T21:03:33Z

lib/Dialect/FIRRTL/Transforms/LowerXMR.cpp

@@ -418,6 +425,62 @@ class LowerXMRPass : public LowerXMRBase<LowerXMRPass> {
    return success();
  }

+  LogicalResult handleTopModuleRefPorts() {


I don't care were they are deleted, as long as they are.

darthscsi · 2023-05-12T21:06:16Z

lib/Dialect/FIRRTL/Transforms/LowerXMR.cpp

+    if (!topModule)
+      return success();


Is this even allowed? Verify seems to say allowed. Spec implies there has to be a real module, but is ambiguous.

darthscsi · 2023-05-12T21:10:11Z

lib/Dialect/FIRRTL/Transforms/LowerXMR.cpp

+      SmallVector<Attribute> refSendPath;
+      SmallString<128> formatString;
+      size_t formatIndex = 0;
+      while (remoteOpPath) {


This needs to be building up a hierpath rather than a series of piece-wise substitutions.

darthscsi · 2023-05-12T21:11:39Z

test/Dialect/FIRRTL/lowerXMR.mlir

+// CHECK-LABEL: sv.macro.decl @ref_Top_Top_a
+// CHECK-LABEL: sv.macro.def @ref_Top_Top_a "{{[{][{]0[}][}]}}" {output_file = #hw.output_file<"ref_Top_Top.sv">, symbols = [#hw.innerNameRef<@Top::@w>]}
+// CHECK-LABEL: sv.macro.decl @ref_Top_Top_b
+// CHECK-LABEL: sv.macro.def @ref_Top_Top_b "{{[{][{]0[}][}]}}.{{[{][{]1[}][}]}}" {output_file = #hw.output_file<"ref_Top_Top.sv">, symbols = [#hw.innerNameRef<@Top::@foo>, #hw.innerNameRef<@Foo::@x>]}


This should be a single substitution of a hierpath. That way the macros are robust to hierarchy manipulation.

Then should I remove the requirement for hierpath's size to be at least 2 (unless I add it outside the circuit op, in which case it's currently not checked at all)? Or should I do the XMRRefOp thing where the symbol can point to a hierpath or be a single inner-ref?

I don't know why we have XMRRefOp separate from hierpath. But you should dig into why hierpath op requires 2, rather than assuming it can't be changed.

Export refs for all public modules. Update macro assembly to explicitly include symbols. Fix paths that end in verbatim expressions.

trilorez · 2023-05-12T23:25:16Z

Updated to handle all public modules.

I also ran into an issue where a path pointing directly to a verbatim expression results in a null inner-ref attribute. I've handled the issue though I think it can come up in other situations such as resolving a reference to a verbatim expression in the same module.

…ath op or containing its own inner ref

trilorez · 2023-05-13T00:12:18Z

Updated to use the XMRRefOp approach towards having either a FlatSymbolRefAttr to a HierPathOp or an InnerRefAttr.

seldridge

LGTM

seldridge · 2023-05-13T00:39:41Z

lib/Conversion/ExportVerilog/ExportVerilog.cpp

+            } else {
+              symVerilogName = namify(sym, symOp);
+            }
+          }


This is a change to the existing verbatim substitution behavior. However, I think this is the right one. If you come across a symbol that is a HierPathOp, then you substitute that path.

Yeah, otherwise it prints the symbol of the HierPathOp itself

This implementation looks good to me. It would be great if we can share the code with visitSV(XMRRefOp) but not within this PR 👍

Please file cleanup issues for stuff like this.

seldridge · 2023-05-13T00:44:20Z

lib/Dialect/FIRRTL/Transforms/LowerXMR.cpp

+      if (failed(resolveReferencePath(portValue, builder, ref, stringLeaf)))
+        return failure();


This will error if the path ends above the current module, yes?

What do you mean by that exactly?

I want to make sure this won't try to generate a path if there is an output reference fed by an input reference that starts "above" the current module.

However, the parser will categorically reject input probes. There shouldn't be a problem here.

A circuit that could lead to weirdness is something like the following. It's the classic "there is no value that you can substitute for this path because it is more than one path" type thing:

circuit Foo : module Bar : input a: Probe<UInt<1>> output b: Probe<UInt<1>> define b = a module Foo : wire x: UInt<1> wire y: UInt<1> inst bar of Bar define bar.a = probe(x) inst bar2 of Bar define bar2.a = probe(y)

seldridge · 2023-05-13T00:45:37Z

lib/Dialect/FIRRTL/Transforms/LowerXMR.cpp

+      // ref_<circuit name>_<module name>_<ref name> <internal path
+      // from module>


Nit: the PR could use shorter names to get this into 80-characters:

Suggested change

// ref_<circuit name>_<module name>_<ref name> <internal path

// from module>

// ref_<circui-namet>_<module-name>_<ref-name> <path>

seldridge · 2023-05-13T00:49:26Z

lib/Dialect/FIRRTL/Transforms/LowerXMR.cpp

+      auto macroName = builder.getStringAttr("ref_" + circuitOp.getName() +
+                                             "_" + module.getName() + "_" +
+                                             module.getPortName(portIndex));


What would happen if the circuit/main module, module, or ref are later renamed? Should this also be renamed or should it not? This is more of a question for the ABI doc about something like:

circuit always: module always: wire module: UInt<1>

Is this: ref_always_always_module module_0, ref_always_0_always_0_module_0 module_0, or something else?

I think it's reasonable to make a case that this follows the original FIRRTL name even if the ref changes. I.e., I think the current code is correct. The spec should probably make this explicit.

seldridge · 2023-05-13T00:54:35Z

test/Dialect/FIRRTL/lowerXMR.mlir

+  // CHECK-NEXT:  sv.macro.def @ref_Top_Top_a "{{[{][{]0[}][}]}}"([#hw.innerNameRef<@Top::@w>]) {output_file = #hw.output_file<"ref_Top_Top.sv">}
+  // CHECK-NEXT:  sv.macro.def @ref_Top_Top_b "{{[{][{]0[}][}]}}"([@xmrPath]) {output_file = #hw.output_file<"ref_Top_Top.sv">}
+  // CHECK-NEXT:  sv.macro.def @ref_Top_Top_c "{{[{][{]0[}][}]}}.internal.path"([#hw.innerNameRef<@Top::@foo>]) {output_file = #hw.output_file<"ref_Top_Top.sv">}
+  // CHECK-NEXT:  sv.macro.def @ref_Top_Top_d "{{[{][{]0[}][}]}}"([#hw.innerNameRef<@Top::@xmr_sym>]) {output_file = #hw.output_file<"ref_Top_Top.sv">}
+  // CHECK-NEXT:  sv.macro.def @ref_Top_Foo_x "{{[{][{]0[}][}]}}"([#hw.innerNameRef<@Foo::@x>]) {output_file = #hw.output_file<"ref_Top_Foo.sv">}
+  // CHECK-NEXT:  sv.macro.def @ref_Top_Foo_y "internal.path" {output_file = #hw.output_file<"ref_Top_Foo.sv">}


Suggestion: CHECK-NEXT-LITERAL would let you match {{0}} directly without the regex escaping.

Informational: I find long matches hard to parse and will often split them up where the output_file is a CHECK-SAME on the next line with indentation. Only informational.

I'm very happy to learn about CHECK-NEXT-LITERAL, {{[{][{]0[}][}]}} looks like a nightmare

Updated to be more readable.
FYI it's CHECK-NEXT{LITERAL}.

include/circt/Dialect/SV/SVStatements.td

uenoku · 2023-05-15T10:14:13Z

lib/Dialect/FIRRTL/Transforms/LowerXMR.cpp

+                             &getContext(), "ref_" + circuitOp.getName() + "_" +
+                                                module.getName() + ".sv"));


nit: could you avoid creating "ref_" + circuitOp.getName() + "_" + module.getName() twice here and in macroName creation?

Co-authored-by: Hideto Ueno <uenoku.tokotoko@gmail.com>

trilorez requested review from seldridge, darthscsi and dtzSiFive May 12, 2023 00:09

trilorez force-pushed the dev/trilorez/export-ref branch from 852b145 to b1a309c Compare May 12, 2023 00:53

seldridge reviewed May 12, 2023

View reviewed changes

darthscsi reviewed May 12, 2023

View reviewed changes

[FIRRTL] Export named references as macros to Verilog files

6a95db3

trilorez force-pushed the dev/trilorez/export-ref branch from b1a309c to 6a95db3 Compare May 12, 2023 19:27

darthscsi reviewed May 12, 2023

View reviewed changes

Add test case for reference to literal

2d680e5

darthscsi reviewed May 12, 2023

View reviewed changes

PR updates

68da70b

Export refs for all public modules. Update macro assembly to explicitly include symbols. Fix paths that end in verbatim expressions.

Use the XMRRefOp approach of having the macro point to either a hierp…

88b1cdc

…ath op or containing its own inner ref

seldridge approved these changes May 13, 2023

View reviewed changes

PR updates

04cac7e

uenoku reviewed May 15, 2023

View reviewed changes

include/circt/Dialect/SV/SVStatements.td Outdated Show resolved Hide resolved

uenoku reviewed May 15, 2023

View reviewed changes

trilorez and others added 2 commits May 15, 2023 08:57

Update include/circt/Dialect/SV/SVStatements.td

6cf6210

Co-authored-by: Hideto Ueno <uenoku.tokotoko@gmail.com>

Reuse circuit module prefix

7e6c361

trilorez merged commit a7e073f into main May 15, 2023

trilorez deleted the dev/trilorez/export-ref branch May 15, 2023 15:30

uenoku mentioned this pull request May 17, 2023

[ExportVerilog] Clean up FlatSymbolRefAttr emission #5217

Open

		if (failed(resolveReferencePath(portValue, builder, ref, stringLeaf)))
		return failure();

		// ref_<circuit name>_<module name>_<ref name> <internal path
		// from module>

	// ref_<circuit name>_<module name>_<ref name> <internal path
	// from module>
	// ref_<circui-namet>_<module-name>_<ref-name> <path>

		&getContext(), "ref_" + circuitOp.getName() + "_" +
		module.getName() + ".sv"));

[FIRRTL] Export named references as macros to Verilog files #5180

[FIRRTL] Export named references as macros to Verilog files #5180

Uh oh!

Conversation

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!