Fix ILA short name

[kleinreact]

Fixes the short name extraction of the ILA cell name to line up with the name, as it is set with Clash's setName.

[martijnbastiaan]

I've done my drive-by review :-). I'll let @hiddemoll be the judge.

[kleinreact]

Unfortunately, the resulting ILA cell names are still a bit unpredictable. fullMeshHwCC produces ilaPlot (set to that value with setName) and fullMeshHwCcTest165___VOID_TDECL_NOOP__ (auto-generated), but boardTestExtended produces boardTestExtended21_boardTestIla, although it is set to only to boardTestIla via setName.

I still think the given update is the correct the way it is, just naming probably has to be fixed/improved within Clash (cf. #2649).

[hiddemoll]

Why would you still let Clash generate ILA names, instead of doing it with setName. I've tried that out locally for fullMeshHwCcTest and fullMeshSwCcTest, in both instances the ILAs get the cell name I expect.

ILA cell name in fullMeshSwCcTest without setName:
fullMeshSwCcTest196_Bittide_Instances_Hitl_FullMeshSwCc_fullMeshSwCcTest_fullMeshSwCcTest197_result_184/fullMeshSwCcTest196___VOID_TDECL_NOOP__
and with setName:
fullMeshSwCcTest196_fincFdecIla/fullMeshSwCcTest196_fincFdecIla

The cell_name for the ILA named ilaPlot is different though, it contains two slashes:
Bittide_Instances_Hitl_FullMeshSwCc_fullMeshSwCcTest_callistoClockControlWithIla_callistoResult/ilaPlot/ilaPlot
I don't understand why that ILA gets a cleaner name.

[kleinreact]

@hiddemoll Is there still any reason to keep this open or can we merge this PR?

[hiddemoll]

We should give all ILAs a specific name in Clash with setName. If we don't, we get ILA cell names with the suffix ___VOID_TDECL_NOOP__.

These are the extracted $short_name in each instance (both fullMesh tests are pretty much identical). The old name is staging, the new name is your branch, the sug is my suggestion. All are after I added the setName for the ILA in fullMeshHwCcTest.

boardTestExtended:

cell: boardTestExtended21_boardTestIla/boardTestExtended21_boardTestIla
old : boardTestIla
new : boardTestExtended21_boardTestIla
sug : boardTestIla

fullMeshHwCcTest:

cell: Bittide_Instances_Hitl_FullMeshHwCc_fullMeshHwCcTest_callistoClockControlWithIla_callistoResult/ilaPlot/ilaPlot
old : Instances_Hitl_FullMeshHwCc_fullMeshHwCcTest_callistoClockControlWithIla_callistoResult
new : ilaPlot
sug : ilaPlot

cell: fullMeshHwCcTest69_fincFdecIla/fullMeshHwCcTest69_fincFdecIla
old : fincFdecIla
new : fullMeshHwCcTest69_fincFdecIla
sug : fincFdecIla

I can't suggest changes to files which you haven't altered, so I've created a new branch: shorter-ila-name. Do you agree on the new short_name which I suggest there?

[kleinreact]

Alright. Then this all just originates from a wrong assumption on the behavior of cell names turning the whole PR obsolete. The only thing that probably should be updated is adding setName to the fincFdeIlas as you did on shorter-ila-name.

I'm still thinking about whether we should enforce the user to provide names for ILAs and VIOs via the API, as currently under discussion here: clash-lang/clash-compiler#2655.

[DigitalBrains1]

If we don't, we get ILA cell names with the suffix ___VOID_TDECL_NOOP__.

A bug which will be fixed by clash-lang/clash-compiler#2662 once I get around to finishing it.

[hiddemoll]

Alright. Then this all just originates from a wrong assumption on the behavior of cell names turning the whole PR obsolete. The only thing that probably should be updated is adding setName to the fincFdeIlas as you did on shorter-ila-name.

That's not all I changed on shorter-ila-name. I also changed how the TCL gets a short_name from the cell name, only a bit differently than you did. You can see that change in the ilaPlot ILA in fullMeshHwCcTest (see my comment above). The short_name for that ILA is definitely not correct on staging.