Improve naming of ILA data dumps

[hiddemoll]

@kleinreact proposed changing the way the Tcl inteprets an ILA name based on the cell-name in Vivado (see discussion on the closed PR #456 ). The goal is to get better file names for the ILA data dumps.

The ilaWb instances in processingElement do not have proper naming, as the setName in the circuit notation seems to have no effect. See issue #530 .

An example of the changes in file names for fullMeshSwCcTest:
Cell names and ILA dump filenames on staging:

Bittide_Instances_Hitl_FullMeshSwCc_fullMeshSwCcTest_callistoClockControlWithIla_callistoResult/ilaPlot/ilaPlot 
Bittide_Instances_Hitl_FullMeshSwCc_fullMeshSwCcTest_fullMeshSwCcTest78_result_227/ila_inst_0 
Bittide_Instances_Hitl_FullMeshSwCc_fullMeshSwCcTest_ilaWb58_result_22/ila_inst 
Bittide_Instances_Hitl_FullMeshSwCc_fullMeshSwCcTest_ilaWb58_result_28/ila_inst

Instances_Hitl_FullMeshSwCc_fullMeshSwCcTest_callistoClockControlWithIla_callistoResult.csv
Instances_Hitl_FullMeshSwCc_fullMeshSwCcTest_fullMeshSwCcTest78_result_227.csv
Instances_Hitl_FullMeshSwCc_fullMeshSwCcTest_ilaWb58_result_22.csv
Instances_Hitl_FullMeshSwCc_fullMeshSwCcTest_ilaWb58_result_28.csv

This branch:

Bittide_Instances_Hitl_FullMeshSwCc_fullMeshSwCcTest_callistoClockControlWithIla_callistoResult/ilaPlot/ilaPlot
fincFdecIla/fincFdecIla
Bittide_Instances_Hitl_FullMeshSwCc_fullMeshSwCcTest_ilaWb58_result_22/ila_inst
Bittide_Instances_Hitl_FullMeshSwCc_fullMeshSwCcTest_ilaWb58_result_28/ila_inst


ilaPlot.csv
fincFdecIla.csv
Instances_Hitl_FullMeshSwCc_fullMeshSwCcTest_ilaWb58_result_22.csv
Instances_Hitl_FullMeshSwCc_fullMeshSwCcTest_ilaWb58_result_28.csv

[kleinreact]

Can you please clarify the question of my comment.

[kleinreact]

I don't get how this matches with the examples of the PR description. As far as I understand these lines of code, they first drop all the prefix up till the last / and then the prefix up till the first _. But then

Bittide_Instances_Hitl_FullMeshSwCc_fullMeshSwCcTest_ilaWb58_result_22/ila_inst

would result in inst.csv and not Instances_Hitl_FullMeshSwCc_fullMeshSwCcTest_ilaWb58_result_22.csv, right?

I guess that I am just missing a bit here. Can you help me out?

[hiddemoll]

You are completely right. This used to be present because of the example I gave here, but I don't see any ILA cell names with the structure {module-name}_{setName}/{module-name}_{setName} which I explained there. I don't really understand why those aren't there anymore. But I will remove the commit where I change the Tcl, and only keep your initial commit.

[hiddemoll]

Neither of our solutions work, specifically because of the 2 ILAs in processingElement. Because they both get called the same name one will overwrite the data dump of the other...

[DigitalBrains1]

I don't really understand why those aren't there anymore.

It might have something to do with a code change which changed the way in which those functions end up being inlined somewhere or not. You can often prevent the prefix with a suitably placed OPAQUE.

As Christiaan said here:

Normalization adds prefixName X when it inlines the function X. The idea was that you could track the provenance of a name, even after inlining.

That’s why the naming mechanism does not ignore these prefixName X ticks. It allows you to distinguish two separate setName Y originating from two different inlined functions.

[hiddemoll]

After a short discussion with @martijnbastiaan I've changed the way we extract a short name from the CELL_NAME of an ILA. Instead of extracting the very last part, I now instead take the last module name. The last part of CELL_NAME is unreliable for the 2 ilaWb instances in processingElement. However, what we can control is the last module name. With these changes the following CELL_NAMEs and data dump files get generated for fullMeshSwCcTest. Note the duplicate names of dataBus, but with a distinct module name.

Bittide_Instances_Hitl_FullMeshSwCc_fullMeshSwCcTest_callistoClockControlWithIla_callistoResult/ilaPlot/ilaPlot
fincFdecIla/fincFdecIla
dataBus/dataBus
instructionBus/dataBus

ilaPlot.csv
fincFdecIla.csv
dataBus.csv
instructionBus.csv

[hiddemoll]

Even after my changes explained in my previous comment the names of ILAs are unstable. My solution works for 3 out of the 4 instances with ILAs which run on staging. On the latest CI run the result was not what I expected for hwCcTopologiesTest, as it produced the following cell name for an ILA which should be named fincFdecIla.

hwCcTopologyTest58_hwCcTopologyTest59_fincFdecIla/hwCcTopologyTest58_hwCcTopologyTest59_fincFdecIla

Although this behaviour is still not what we want, I think it is an upgrade over what is happening on staging. So I think this PR could still be merged. If needed I can post a list of the ILA data dump file names before and after this PR.

[martijnbastiaan]

So I think this PR could still be merged.

I'm in favor of PRs slightly improving the status quo btw. We don't have to go all the way for things to be useful.

[martijnbastiaan]

This seems like the way to go. It's a bit tough to see how hard / unexpected naming behaves in Clash though. This still needs to be paired with strategic {-# OPAQUE ... #-}s right?

Also, is this now affected by clash-lang/clash-compiler#2722 or not @hiddemoll?

[kleinreact]

LGTM now.

[hiddemoll]

This seems like the way to go. It's a bit tough to see how hard / unexpected naming behaves in Clash though. This still needs to be paired with strategic {-# OPAQUE ... #-}s right?

Adding {-# OPAQUE ... #-} for the functions where an ILA results in compile errors because probe names cannot be matched, like what happens here.

Clash error call:
    Tried create a signal called 'trigger_0', but identifier generation returned 'trigger_0_0
    '. Refusing to instantiate Ila with unreliable probe names.

Also, is this now affected by clash-lang/clash-compiler#2722 or not @hiddemoll?

I don't think our HITL infrastructure, where the names of certain VIO and ILA probes are hardcoded, was ever not dependent on that issue.

[martijnbastiaan]

    Tried create a signal called 'trigger_0', but identifier generation returned 'trigger_0_0
    '. Refusing to instantiate Ila with unreliable probe names.

Should be an option to turn this off IMO. Our TCL doesn't rely on this.