[JasmijnB]

Part of #914

Removes the VHDLSignal classes in utils.py and replaces all occurrences with the Signals class.
The classes are almost the exact same, with exception of a slight difference in the naming convention. So to make this compatible, a dyn_comp flag is added to the Signals classes and enabled for all the replaced signals.
This change also requires the introduction of the Context class in the wrapper generator, to be consistent with the remainder of the LSQ generation.

As I was unable to cherry pick the changes (the original branch has a different approach order) I've let Claude do the rewrite based on the original branch and checked this

Sorry for the delay in merging this!

[Jiahui17]

What does dyn comp do?

[JasmijnB]

It modifies the signal names such that they are compatible with what the compiler expects.
This changes two things:

• It drops the _i and _o from the input and output signal names
• It moves the index one word ahead (e.g. io_ctrl_0_ready instead of io_ctrl_ready_0)

This was essentially the only difference between the two signal classes, the now-removed VHDLSignal behaves as Signal with dyn_comp = true.
Maybe there's a way to just adjust the dynamatic side so we don't need to have this flag, but I would put this in a separate PR.

[Jiahui17]

Shouldn't we always use this "more correct" signal naming instead of adding a separate switch for it?

[JasmijnB]

Personally I prefer the current way of doing it: the _i and _o (together with the register _q and _d) make things more readable in the output code.

The numbering before the last word only works for specific signal names, e.g. it's nice for ctrlEnd_[x]_ready, but changing store_is_older_[x] into store_is_[x]_older doesn't work. In addition, what do you do if there are variable names without an _?

So I would either keep this flag just to be used in the wrapper, or change dynamatic side to work with the current signals implementation (which I think should be a separate PR)

[JasmijnB]

Okay I've thought about it a bit more and I'm in favor of just keeping the flag, the diff between dyn and the LSQ is that all dynamatic signals are handshake signals, so there it is nice to have the number before the flag (as every signal has a handshake _ready, _valid and _data), in addition, there's not much of an advantage to having _i and _o as this is already part of the handshake protocol.
This is not the case for the LSQ, so doing this makes no sense for all other non-elastic variable names. Thus the interface has a different naming scheme than the internals.

[Jiahui17]

Isn't it cleaner to let the wrapper handle all the naming differences? Or sometimes we don't instantiate this wrapper...?

[JasmijnB]

How would you go about this?
Currently only the wrapper is setting dyn_comp = True, and only for the output ports (this was the only code using the old VHDLSignals)

I'm not entirely sure how to move the code for the different naming convention out of the Signals class without doing big string refactors at the end of the wrapper (after it generated the entire source string) which I think will get really ugly

[Jiahui17]

I think if we really want to emphasize the port type (e.g., if it needs to have suffices like "_o" or "_i"), they should already appear in the signal names

For example, i found this line in the code:

 io_stDataToMC_valid = Logic(ctx, "io_stDataToMC_valid", 'o', dyn_comp=True)

Here we already had a prefix io_* for all the interface signals.

we could do

 output_stDataToMC_valid = Logic(ctx, "output_stDataToMC_valid", 'o')

[JasmijnB]

In practice this is already happening, here's a snippet from group_allocator:

        ldq_tail_i = LogicVec(em, "ldq_tail", "i", self.configs.ldqAddrW)
        ldq_head_i = LogicVec(em, "ldq_head", "i", self.configs.ldqAddrW)
        ldq_empty_i = Logic(em, "ldq_empty", "i")

        stq_tail_i = LogicVec(em, "stq_tail", "i", self.configs.stqAddrW)
        stq_head_i = LogicVec(em, "stq_head", "i", self.configs.stqAddrW)
        stq_empty_i = Logic(em, "stq_empty", "i")

        ldq_wen_o = LogicArray(em, "ldq_wen", "o", self.configs.numLdqEntries)
        num_loads_o = LogicVec(em, "num_loads", "o", self.configs.ldqAddrW)
        num_loads = LogicVec(em, "num_loads", "w", self.configs.ldqAddrW)

I suppose we could manually add all the _i and _o to all the signal strings and then just leave them out in the wrapper?

The specific change you're suggesting is one of the interface signals, which I would say "io" is fine for because they're all handshake signals (also I think adding separate "input_" and "output_" on the compiler side will be annoying)

Then we still have the number ordering issue, what would you do for that? I'm personally against moving the number before the last word in every single signal name as it feels very unintuitive.
I haven't looked into what to do on the compiler side to move the numbers to the end, and if all the other interfaces have the same [var name]_[number]_[ready|valid|data] convention I'd say it's not good to change that just for the LSQ

[Jiahui17]

the numbers go before valid|ready everywhere so it is better to just change LSQ

[JasmijnB]

Alright wait maybe another option is to have a new optional parameter called custom_suffix
By default the suffix is _i/_o/_w based on the type of the signal
Then for the wrapper we set the custom suffix to be _ready/_valid etc.

As suffixes are appended after the signal base name, this fixes both problems

If that's an okay solution I can implement that?

[Jiahui17]

Isn't it the same as adding the suffix directly into the signal name...?

[JasmijnB]

It's different because if you add suffixes directly to the signal name we still have the numbers-before-last-word issue.

E.g. if I have LogicArray(ctx, "ldq_data_valid", "r", self.configs.numLdqEntries ), this needs to become ldq_data_0_valid, ldq_data_1_valid etc. in order to interface with the compiler correctly.

However, internally in the LSQ I would say putting the number before the last word makes no sense at all, e.g. in the store_is_older wire etc.
So then you'd add the number at the end if the type is a wire/register, and add a number before the last word if the type is an input/output and assume that the user follows convention?

[Jiahui17]

Maybe the easiest is to not use LogicArray for signals that interface with the dataflow circuit and just use a Python array instead:

valids = []

for id_, s in enumerate(...):
  valids.emplace_back("ldq_data_{id_}_valid")

[JasmijnB]

To be really honest, I think the flag is easier than having to extend every input/output signal name and rewrite the lsq.py wrapper file to use these arrays.

A flag to generate dynamatic compatible names makes sense to me, as it's just a different naming scheme being applied for the wrapper. In addition, having programmed using the current library, I think the current suffixes are nicer than having to write ...("[signal name]_i", "i"...), the extra text doesn't really add readability as you're using the python variable names anyways.

But if you've got a strong opinion on this, I could change it :) I would prefer to first merge the full branch though as this will make cherry picking my original changes impossible, if that's not a big issue

[Jiahui17]

So custom suffix is kind of the simplest workaround? Just want to double check: this is only for the wrapper, right?

[JasmijnB]

Yes I believe the custom suffix is cleanest and easiest, and it's only for the wrapper. I'll update the implementation as soon as I have time!

[Jiahui17]

Okay sounds good! Let's go with that

[JasmijnB]

Okay I updated the code!

[JasmijnB]

I think the integration tests failing is a more general issue? I'm personally unable to merge because I don't have the rights

[Jiahui17]

Should rebase first (your branch doesn't have a few commits in the main that improve the performance

[JasmijnB]

Rebased!