Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
TommyMurphyTM1234 opened this issue Jul 2, 2024 · 2 comments

Comments

@TommyMurphyTM1234
Copy link
Contributor

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
Copy link
Collaborator

aap-sc commented Jul 2, 2024

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.

  • 32-bit platforms:

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.

  • 64-bit platforms:

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
Copy link
Collaborator

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants