circt icon indicating copy to clipboard operation
circt copied to clipboard

[Python][HW] hw.WireOp inner_sym is inconvenient to use

Open nickelpro opened this issue 1 year ago • 1 comments

The move to using InnerSymAttr in https://github.com/llvm/circt/pull/5703 broke a somewhat obscure usage of the Python API for creating hw.WireOps directly.

The old code used to look like this:

op = hw.ConstantOp(ir.BoolAttr.get(True))
wire_name = "Example"
wire = hw.WireOp(op, name=wire_name, inner_sym=wire_name)

Now we have:

op = hw.ConstantOp(ir.BoolAttr.get(True))
wire_name = "Example"
wire = hw.WireOp(op, name=wire_name,
                 inner_sym=hw.InnerSymAttr.get(ir.StringAttr.get(wire_name)))

Which doesn't seem better.

sv.WireOp() got a sym_name parameter that does the right thing:

if sym_name is not None:
  attributes["inner_sym"] = hw.InnerSymAttr.get(StringAttr.get(sym_name))

The same fix should probably be applied to WireOp, and maybe just generally to all hw operations

nickelpro avatar Apr 25 '24 23:04 nickelpro

Thanks for bringing it up. We generally have convenience helpers in the Python operation builder APIs that take, for example, a Python string, and build the appropriate attribute. Like you've pointed out for sv.WireOp, which is defined here: https://github.com/llvm/circt/blob/ce9877c55544937aec68b2a5b099ccef788168ba/lib/Bindings/Python/dialects/sv.py#L35.

We simply don't have any convenience helper around the default generated Python operation builder for hw.WireOp, so the user is required to manually construct the appropriate attributes. I think it would be very reasonable to have this for hw.WireOp, so the user can supply inner_sym as a Python string, and the helper would construct the appropriate InnerSymAttr.

mikeurbach avatar Apr 26 '24 00:04 mikeurbach