SystemVerilog wire names

In the ODM today, I was talking about things like:

hw.module @test (%a: i32) -> (%x: i32, %y: i32) {
  %0 = comb.or %a, %a {name="testname"} : i32
  hw.output %0, %0 : i32, i32
}

which generates:

module test(
  input  [31:0] a,
  output [31:0] x, y);

  wire [31:0] testname = a | a;
  assign x = testname;
  assign y = testname;
endmodule

This would drastically reduce the complexity of the IR in the example shown today. The downside is that the testname wire is not guaranteed to appear … that expression could (and very likely would be in the comb.or case) inlined. Further, the inlining heuristics can (and do) change so when you bump the CIRCT version you can have different wire names. I have found that if I name all the comb/hw/seq ops, I get well-named SV wires (vs _T* names) while allowing ExportVerilog to inline expressions in a very reasonable way (IMO). I suspect this is what the Raj and Lenny actually want.

That said, if you need a wire to show up in your SV output (to get parity with their existing SV output), sv.wire is the only way to guarantee that. You can also use a combination of the two approaches.

I apologize for being unclear in the meeting today.

Aha, I see what you mean. It looks like this is an artifact of a bug in exportverilog. This code isn’t checking for declarations:

   if (!names.hasName(result))
        names.addName(result, op.getAttrOfType<StringAttr>("name"));

Two thoughts:

  1. I think we should remove this behavior. This isn’t something we can maintain or support (e.g. canonicalizations and other things will just drop the ‘name’ field) so it won’t be very useful in practice for frontends to generate. This is similar in some ways to LLVM’s known-problematic “metadata” fields which are dropped all the time. This model hasn’t worked out well.

  2. We definitely shouldn’t recommend this to frontend authors who want predictable names!

Does anyone object to me just fixing this bug, removing this?

-Chris

I object pretty strongly! This is immensely useful for creating readable SystemVerilog, even if it’s not guaranteed. Even if optimization passes don’t copy the names, it’s useful. Anything which operates down the stack could (and does) make use of it.

Two examples:

  1. ESI uses it extensively in creating the Capnp shims. This results in well-named SV which is possible to debug, even though it uses the comb operations. I don’t need the names to be consistent and stable. I just need them to be relatively descriptive and not _T*.
  2. PyCDE provides the ability to name any operation and does it’s best to chain reasonable names on operations which use the Value produced by a named operation. (e.g. if array_create {name='foo'} followed by array_get %arr[%c0] {name = "foo_at_0}, the ‘foo_at_0’ is auto-generated). This was a major selling point. Again, I don’t need the names to be consistent or stable and I do want to allow all the reasonable inlining logic in ExportVerilog to create reasonable SV, so I don’t want to create a bunch of sv.wires.

In neither case do I need the names to be consistent, stable, or respected.

I think we’ve discussed this in the past and again, having “name hints” on operations is a major feature for me. IMO, it is the most reasonable way to create reasonable SV (i.e. making use of the ExportVerilog heuristics) if one is working at a lower level or producing ‘debug’ mode code (No optimizations). If optimizations destroy those names, that’s reasonable and kind of expected since optimizations are well known to produce sort of ugly, difficult-to-debug code.

If there’s a better way to achieve what I need, I’m all ears but please don’t eliminate this functionality in the meantime. (Perhaps a pass which contains heuristics to produce sv.wires…?) It would be a pretty significant regression in the SV readability.

Sure, I won’t unilaterally remove it (that’s why I asked :slight_smile: ).

I don’t think that foo_at_0 is a great example for this – we should directly improve the verilog emitter to synthesize symbolic names like this, not rely on “some pass” doing it ahead of time and relying on it to be stable.

I added it to the agenda for discussion next week, I’d love it if you could put together some more information about concrete use cases so we can discuss various implementation approaches for them. Thanks!

-Chris

Thanks. I may have overreacted a bit.

I agree – the emitter could be better about naming stuff, but I don’t think it has enough information (currently) to create wire names which are relevant to the application code. And again, the names don’t have to be stable – this is just for debugging, no external tools should rely on the wire name.

I don’t know if I’ll have time before the next one, but I can put some together within the next few weeks.

The primary use-case is debugging via waveforms. I’ve found it useful in debugging both ESI and an internal project which uses PyCDE. Below is the ESI example and I’ve attempted to put together a demonstrative example in PyCDE.

ESI Debugging

During the ESI cosim development (specifically, building the Capnp gaskets) I spent a lot of time staring at waveforms named _T_*. So, I added a helper to name hw and comb ops. I call that function everywhere (e.g. here, here, here). For values which are simple derivations, I automatically name them (e.g. here). This does not create terribly pretty wire names and it doesn’t get everything (I added names until I found and fixed my bugs), but they’re a helluva lot better than _T_*:

Without debug names:

module decodeArrayOf4xSi64(     // <stdin>:114:3
  input              clk, valid,
  input  [383:0]     encodedInput,
  output [3:0][63:0] decoded);

  wire [63:0] _T = encodedInput[9'h0+:64];      // <stdin>:115:14, :116:10
  wire [31:0] _T_0 = _T[6'h0+:32];      // <stdin>:117:14, :118:10, :119:10
  wire [15:0] _T_1 = _T[6'h20+:16];     // <stdin>:120:16, :121:10, :122:10
  wire [15:0] _T_2 = _T[6'h30+:16];     // <stdin>:123:16, :124:10, :125:10
  wire [63:0] _T_3 = encodedInput[9'h40+:64];   // <stdin>:128:17, :129:10
  wire [63:0] _T_4 = _T_3[6'h0+:64];    // <stdin>:130:16, :131:10
  wire [1:0] _T_5 = _T_4[6'h0+:2];      // <stdin>:132:16, :133:11, :141:11
  wire [2:0] _T_6 = _T_4[6'h20+:3];     // <stdin>:137:18, :138:11, :142:11
  wire [28:0] _T_7 = _T_4[6'h23+:29];   // <stdin>:139:16, :140:11, :143:11
  wire [35:0] _T_8 = {_T_4[6'h2+:30], 6'h0} + 36'h80;   // <stdin>:134:14, :135:11, :136:11, :144:16, :145:11, :146:17, :147:11
  wire [8:0] _T_9 = _T_8[8:0];  // <stdin>:148:11
  wire [3:0][63:0] _T_10 = /*cast(bit[3:0][63:0])*/encodedInput[_T_9+:256];     // <stdin>:149:11, :150:11
  wire [63:0] _T_11 = _T_10[2'h0];      // <stdin>:151:14, :152:11, :153:11
  wire [63:0] _T_12 = _T_10[2'h1];      // <stdin>:158:14, :159:11, :160:11
  wire [63:0] _T_13 = _T_10[2'h2];      // <stdin>:165:15, :166:11, :167:11
  wire [63:0] _T_14 = _T_10[2'h3];      // <stdin>:172:15, :173:11, :174:11
  always @(posedge clk) begin   // <stdin>:180:5
    if (valid) begin    // <stdin>:181:7
      assert(_T_0 == 32'h0);    // <stdin>:182:19, :183:15, :184:9
      assert(_T_1 == 16'h0);    // <stdin>:185:19, :186:15, :187:9
      assert(_T_2 == 16'h1);    // <stdin>:188:19, :189:15, :190:9
      assert(_T_5 == 2'h1);     // <stdin>:191:20, :192:15, :193:9
      assert(_T_6 == 3'h5);     // <stdin>:194:19, :195:15, :196:9
      assert(_T_7 <= 29'h4);    // <stdin>:197:19, :198:15, :199:9
    end
  end // always @(posedge)
  assign decoded = {{{_T_11[63], _T_11[62:0]}}, {{_T_12[63], _T_12[62:0]}}, {{_T_13[63], _T_13[62:0]}}, {{_T_14[63], _T_14[62:0]}}};    // <stdin>:154:11, :155:11, :156:11, :157:11, :161:11, :162:11, :163:11, :164:11, :168:11, :169:11, :170:11, :171:11, :175:11, :176:11, :177:11, :178:11, :179:11, :202:5
endmodule

With some debug names:

module decodeArrayOf4xSi64(     // <stdin>:114:3
  input              clk, valid,
  input  [383:0]     encodedInput,
  output [3:0][63:0] decoded);

  wire [63:0] rootPointer = encodedInput[9'h0+:64];     // <stdin>:115:14, :116:10
  wire [31:0] typeAndOffset_casted = rootPointer[6'h0+:32];     // <stdin>:117:14, :118:10, :119:10
  wire [15:0] dataSectionSize = rootPointer[6'h20+:16]; // <stdin>:120:16, :121:10, :122:10
  wire [15:0] ptrSectionSize = rootPointer[6'h30+:16];  // <stdin>:123:16, :124:10, :125:10
  wire [63:0] ptrSection = encodedInput[9'h40+:64];     // <stdin>:128:17, :129:10
  wire [63:0] l_ptr = ptrSection[6'h0+:64];     // <stdin>:130:16, :131:10
  wire [1:0] l_ptrType_casted = l_ptr[6'h0+:2]; // <stdin>:132:16, :133:11, :141:11
  wire [2:0] l_elemSize_casted = l_ptr[6'h20+:3];       // <stdin>:137:18, :138:11, :142:11
  wire [28:0] l_listLength_casted = l_ptr[6'h23+:29];   // <stdin>:139:16, :140:11, :143:11
  wire [35:0] l_listOffset = {l_ptr[6'h2+:30], 6'h0} + 36'h80;  // <stdin>:134:14, :135:11, :136:11, :144:16, :145:11, :146:17, :147:11
  wire [8:0] _T = l_listOffset[8:0];    // <stdin>:148:11
  wire [3:0][63:0] list_casted = /*cast(bit[3:0][63:0])*/encodedInput[_T+:256]; // <stdin>:149:11, :150:11
  wire [63:0] _T_0 = list_casted[2'h0]; // <stdin>:151:14, :152:11, :153:11
  wire [63:0] _T_1 = list_casted[2'h1]; // <stdin>:158:14, :159:11, :160:11
  wire [63:0] _T_2 = list_casted[2'h2]; // <stdin>:165:15, :166:11, :167:11
  wire [63:0] _T_3 = list_casted[2'h3]; // <stdin>:172:15, :173:11, :174:11
  always @(posedge clk) begin   // <stdin>:180:5
    if (valid) begin    // <stdin>:181:7
      assert(typeAndOffset_casted == 32'h0);    // <stdin>:182:19, :183:15, :184:9
      assert(dataSectionSize == 16'h0); // <stdin>:185:19, :186:15, :187:9
      assert(ptrSectionSize == 16'h1);  // <stdin>:188:19, :189:15, :190:9
      assert(l_ptrType_casted == 2'h1); // <stdin>:191:20, :192:15, :193:9
      assert(l_elemSize_casted == 3'h5);        // <stdin>:194:19, :195:15, :196:9
      assert(l_listLength_casted <= 29'h4);     // <stdin>:197:19, :198:15, :199:9
    end
  end // always @(posedge)
  assign decoded = {{{_T_0[63], _T_0[62:0]}}, {{_T_1[63], _T_1[62:0]}}, {{_T_2[63], _T_2[62:0]}}, {{_T_3[63], _T_3[62:0]}}};    // <stdin>:154:11, :155:11, :156:11, :157:11, :161:11, :162:11, :163:11, :164:11, :168:11, :169:11, :170:11, :171:11, :175:11, :176:11, :177:11, :178:11, :179:11, :202:5
endmodule

I could have added names in more places, but I found the bugs without any more names.

PyCDE

Given this program snippet:


@module
class WireNames:
  clk = Input(types.i1)
  sel = Input(types.i2)
  data_in = Input(dim(32, 3))

  a = Output(types.i32)
  b = Output(types.i32)

  @generator
  def build(mod):
    foo = mod.data_in[0]
    foo.name = "foo"
    arr_data = dim(32, 4).create([1, 2, 3, 4], "arr_data")
    return {
        'a': foo.reg(mod.clk).reg(mod.clk),
        'b': arr_data[mod.sel],
    }

With naming disabled in PyCDE:

module pycde_WireNames( // /home/jodemme/circt/frontends/PyCDE/test/verilog_readablility.py:16
  input              clk,
  input  [2:0][31:0] data_in,
  input  [1:0]       sel,
  output [31:0]      a, b);

  reg [31:0] _T;        // /home/jodemme/circt/frontends/PyCDE/test/verilog_readablility.py:16
  reg [31:0] _T_0;      // /home/jodemme/circt/frontends/PyCDE/test/verilog_readablility.py:16

  wire [3:0][31:0] _T_1 = {{32'h4}, {32'h3}, {32'h2}, {32'h1}}; // /home/jodemme/circt/frontends/PyCDE/test/verilog_readablility.py:16
  always @(posedge clk) begin   // /home/jodemme/circt/frontends/PyCDE/test/verilog_readablility.py:16
    _T <= data_in[2'h0];        // /home/jodemme/circt/frontends/PyCDE/test/verilog_readablility.py:16, :17
    _T_0 <= _T; // /home/jodemme/circt/frontends/PyCDE/test/verilog_readablility.py:16
  end // always @(posedge)
  assign a = _T_0;      // /home/jodemme/circt/frontends/PyCDE/test/verilog_readablility.py:16
  assign b = _T_1[sel]; // /home/jodemme/circt/frontends/PyCDE/test/verilog_readablility.py:16, :22
endmodule

With naming re-enabled, PyCDE generates this IR:

  hw.module @pycde.WireNames(%clk: i1, %data_in: !hw.array<3xi32>, %sel: i2) -> (%a: i32, %b: i32) {
    %c0_i2 = hw.constant 0 : i2
    %0 = hw.array_get %data_in[%c0_i2] {name = "foo"} : !hw.array<3xi32>
    %c1_i32 = hw.constant 1 : i32
    %c2_i32 = hw.constant 2 : i32
    %c3_i32 = hw.constant 3 : i32
    %c4_i32 = hw.constant 4 : i32
    %1 = hw.array_create %c4_i32, %c3_i32, %c2_i32, %c1_i32 : i32
    %2 = seq.compreg %0, %clk {name = "foo__reg1"} : i32
    %3 = seq.compreg %2, %clk {name = "foo__reg2"} : i32
    %4 = hw.array_get %1[%sel] : !hw.array<4xi32>
    hw.output %3, %4 : i32, i32
  }

Which results in this SV:

module pycde_WireNames( // /home/jodemme/circt/frontends/PyCDE/test/verilog_readablility.py:16
  input              clk,
  input  [2:0][31:0] data_in,
  input  [1:0]       sel,
  output [31:0]      a, b);

  reg [31:0] foo__reg1; // /home/jodemme/circt/frontends/PyCDE/test/verilog_readablility.py:16
  reg [31:0] foo__reg2; // /home/jodemme/circt/frontends/PyCDE/test/verilog_readablility.py:16

  wire [3:0][31:0] arr_data = {{32'h4}, {32'h3}, {32'h2}, {32'h1}};     // /home/jodemme/circt/frontends/PyCDE/test/verilog_readablility.py:16
  always @(posedge clk) begin   // /home/jodemme/circt/frontends/PyCDE/test/verilog_readablility.py:16
    foo__reg1 <= data_in[2'h0]; // /home/jodemme/circt/frontends/PyCDE/test/verilog_readablility.py:16, :17
    foo__reg2 <= foo__reg1;     // /home/jodemme/circt/frontends/PyCDE/test/verilog_readablility.py:16
  end // always @(posedge)
  assign a = foo__reg2; // /home/jodemme/circt/frontends/PyCDE/test/verilog_readablility.py:16
  assign b = arr_data[sel];     // /home/jodemme/circt/frontends/PyCDE/test/verilog_readablility.py:16, :22
endmodule

If, in the future, CIRCT gets an optimization which detects shift registers and combines foo__reg1 and foo__reg2 to reg [31:0] [1:0] _T, that’s fine. Using sv.wire would prevent said theoretical optimization, so I don’t want PyCDE emitting that.


This is the process I see a designer going through in the very short term: looking at random names in a waveform viewer, then adding meaningful names where ever helps for debugging. Don’t get me wrong – this is not ideal. Really, really not ideal. But it beats _T_*. Think of it as an escape hatch.

Ideally, we’d get variable names from the designer’s source and include them as name attributes. (This is on my list for PyCDE, actually.) In a hypothetical “debug” mode, a large portion of them would be emitted. If any optimizations are enabled, it’s just best-effort – no guarantees.

In either case, however, letting a frontend give wire name “hints” is useful – be it through the name attribute or using some other mechanism.

1 Like

Fabian suggests we could do something like:

hw.module @test (%a: i32) -> (%x: i32, %y: i32) {
  %0 = comb.or %a, %a : i32
  %1 = hw.name "testname" %0 : i32
  hw.output %1, %1 : i32, i32
}

Does this guarantee that a name get emitted (mean, preventing inlining, preventing folding etc), or does it work similar to the existing “name attribute on a comb node” thing? Both are useful.

Or keep it out of the SSA use-def edges:

hw.module @test (%a: i32) -> (%x: i32, %y: i32) {
  %0 = comb.or %a, %a : i32
  hw.probe "testname" %0 : i32    // hw.probe would have side effects.
  hw.output %0, %0 : i32, i32
}

… this will affect the generated code less, but will still change things, because anything depending on “has one use” will see the probe as a user.

We had a nice long discussion in the CIRCT design meeting this week.

[HW] Implement hw.name to suggest wire names · Issue #1752 · llvm/circt (github.com) I renamed “probe” to “name” since “probe” implies some sort of permanence (i.e. must expose a wire of this name which carries the same value as this Value) whereas “name” doesn’t.