riscv-software-src / riscv-tests

Other
824 stars 438 forks source link

[Debug] why are Spike target ISA/architecture specifiers inconsistent? #568

Open TommyMurphyTM1234 opened 3 days ago

TommyMurphyTM1234 commented 3 days ago

I noticed this when dealing with this:

The Spike targets for debugging use different ISAs/architecture specifications:

Is there a reason for the lack of consistency here? Should there be more consistency?

aap-sc commented 3 days ago
Is there a reason for the lack of consistency here?
Should there be more consistency?

Yes and no :) . It's a complicated topic.

We can go on case-by-case basis. The thing is that there is lot's of configuration options that affect OpenOCD behavior. I guess that the diversity in configuration is not a bad thing. For example in Syntacore we have like 20 configurations based on spike to test OpenOCD. I doubt that we need such diversity in open source, though.

That being said I guess that these configurations could use better naming instead of general "spike32"/"spike64" and the like. It also does not help that aside from ISA string we have difference in non-isa stuff (like hasel) which is not reflected in any way in platform description. Changing this naming scheme will cause compatibility problems for users of this testsuite. If there is a need or desire to switch to a new naming scheme I would prefer to get an explicit approval from @aswaterman before addressing this.

Now, going case-by-case.

platform ISA non-isa V D C
spike-multi.py RV32IMAFDCV + + +
spike32-2-hwthread.py RV32IMAFDV hasel + + -
spike32-2.py RV32IMAFC progbufsize=0, dmi_rti=4 - - +
spike32.py RV32IMAFDCV dmi_rti=4 + + +

C, D, V extensions affect how OpenOCD behaves in certain scenarios. So having platforms that toggle this support help to increase coverage. Obviously, some care is needed to cherry-pick only "inserting" combinations (or you get a combinatorial explosion) . The ones we have now are not THAT bad.

I guess the same sitation. My guess (never bothered to check) that the ones that you list as "None" are derived from misa directly.

In the end it's up to the users to decide which combinations they need. If someone wants to have a specific combination to be tested - they can create MR.

TLDR:

  1. This diversity in configuration is fine in my opinion.
  2. We can do the renaming, but I would like to get an approval from a broader audience (@aswaterman and the like)
  3. New platform should definitely use more verbose naming scheme and we should review relevant changes with more scrutiny.
aswaterman commented 2 days ago

I don't object to renaming these targets. I leave the decision to y'all.

But note that Spike does rely on their names in its CI flow: https://github.com/riscv-software-src/riscv-isa-sim/blob/1b1a333763eae2e74dbf38b39d9adab39c4bed7c/.github/workflows/debug-smoke.yml

So, if we do rename them, please make a corresponding PR to Spike that updates the riscv-tools commit hash (line 50) and the filenames (lines 55 and 60).