[VHDL/Backend] Incompatible signal types for 1-bit address

[Jiahui17]

Commit: bdfc973d4ed46ff2b3ac5765c2063543b6425a9a

To reproduce the issue:

dynamatic> set-dynamatic-path ./dynamatic;   set-src ./dynamatic/integration-test/admm/admm.c;   set-clock-period 8;   compile --buffer-algorithm on-merges;   write-hdl --hdl vhdl;   simulate;   exit

---

Description: https://github.com/EPFL-LAP/dynamatic/blob/bdfc973d4ed46ff2b3ac5765c2063543b6425a9a/integration-test/admm/admm.c#L5-L8

The out signal has 2 entries, which can be represented using 1-bit.

---

Incompatible port type in admm_wrapper.vhd vs hls_verify_admm_tb.vhd: In the generated admm_wrapper.vhd, the address port has type std_logic:

    out_address0 : out std_logic;

Whereas in the wrapper generated by hls-verifier, the address port has type std_logic_vector (1 downto 0) (ADDR_WIDTH_out = 1):

    signal out_mem_address0 : std_logic_vector(ADDR_WIDTH_out - 1 downto 0);

---

Cause: The kernel interface is specified as follows in the HW dialect:

out out_loadAddr : i1,

Here, out_loadAddr is not a handshake type but a plain integer type, so the logic here interprets the signal as a std_logic: https://github.com/EPFL-LAP/dynamatic/blob/bdfc973d4ed46ff2b3ac5765c2063543b6425a9a/tools/export-rtl/export-rtl.cpp#L569-L571

And the signal is always an array in VHDL.

---

Discussion: Should we attach an additional attribute to all module port signals to indicate if a signal is a word or just a control bit (a word may or may not be 1-bit, but a signal is always 1-bit)?

By default, an interface signal is a word

• VHDL: std_logic_vector(DATA_TYPE - 1 downto 0)
• Verilog : [DATA_TYPE-1:0] wire;

Another solution is to force all signals with integer/float types to be std_logic_vector

[lucas-rami]

Thanks for reporting the issue and for detailing the problem so nicely. I think attaching an attribute to ports which are 1-bit but could technically be vectors (like an address bus, but not like a valid signal) makes a lot of sense. That would only require modifications in the HandshakeToHW pass and export-rtl tool I think. The first would set the attribute on some ports, and the second would just read them to know which RTL type to print.

[Jiahui17]

https://github.com/EPFL-LAP/dynamatic/blob/107e3e466b689be04baca0c427fd7eeefa447825/lib/Conversion/HandshakeToHW/HandshakeToHW.cpp#L263-L265

Does it make sense to change the addrType here to hw::Int to differentiate with a built-in MLIR int?

https://github.com/EPFL-LAP/dynamatic/blob/107e3e466b689be04baca0c427fd7eeefa447825/include/dynamatic/Dialect/HW/HWTypesImpl.td#L29-L54

Here it says:

This type represents the case when the width is a parameter in the HW dialect sense.

Maybe the intention here is that the int width is meant to be variable; so when lowering this thing to an RTL, it has to be a vector type (e.g., std_logic_vector (width - 1 downto 0) in vhdl)

[lucas-rami]

After giving this some more thought, I don't think we should bog down the IR with this information, given that a signal being a 1-bit type or a vector it a property of the RTL component being instantiated and not of the abstract representation of the component it belongs to in the IR.

To illustrate what I am saying, consider a component's signal that may have any non-zero width. Let's say that there are two RTL implementations of that component for some reason, one optimized implementation when that signal has width 1 and a generic implementation for other widths. We probably want to use a 1-bit type like std_logic for the signal in the optimized implementation, since it only works with that width. On the other hand, we have to use a vector type like std_logic in the generic implementation. The existence of this decision and of the two RTL implementation is opaque to the compilation flow down to and including HW, which only care about component's interface, not their final type in the RTL. Hence this information has to be encoded in the RTL configuration.

I think a new JSON parameter similar to the io-map parameter and which let people override whatever default behavior we define on a per-signal-name basis would work fine. Let me know if you think this is a good idea and I can try to give this a go at some point (probably not very quickly unfortunately, but in the coming weeks).

[Jiahui17]

Thanks, now I understand what you meant by attaching an attribute (I thought you meant an attribute in the hw operation).

Would it be something like:

  {
    "name": "handshake.mem_controller",
    "io-width": [
      { "*loadAddr*" : "variadic" }
      { "*loadEn*" : "1" }
    ],
    "generic": "$DYNAMATIC/data/vhdl/handshake/mem_controller_storeless.vhd",
    "dependencies": ["mc_support"]
  },

The logic would be:

1. If the "io-width" entry presents and has a concrete value (e.g., 1 for loadEn), then the hw operation must match exactly the width specified in the RTL entry.
2. If the "io-width" entry presents and is "variadic", then the hw operation will match and the corresponding signal must be a vector type even if the width is 1.
3. If the "io-width" is absent, then the signal type is inferred automatically (always a vector if it has > 1 bits, otherwise a signal bit).

I tried to make this attribute a bit more useful for other things (e.g., we only support 32-bit FP adders, so the RTL generator would not claim there is a matching RTL entry for a custom 20-bit FP adder...

[lucas-rami]

Thanks, now I understand what you meant by attaching an attribute (I thought you meant an attribute in the hw operation).

It's because that is what I meant initially before realizing I did not like the idea so much ><

Would it be something like:

Thanks for the proposal, it's basically what I meant yes. I think your "concrete value" replicates functionality that's already baked in the RTL config though. For example, the bitwidth restriction on an FP adder would already be encoded in RTL parameters, so there is no need to be able to express it here.

{
  "name": "handshake.addf",
  "parameters": [
      { "name": "DATA_TYPE", "type": "dataflow", "data-eq": 32}
  ]
}

To me it seems that a true/false value for each signal name pattern is enough then; false being the default and meaning "always a vector if it has > 1 bits, otherwise a signal bit" and true meaning "the corresponding signal must be a vector type even if the width is 1". Does that seem ok?