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

Use of pseudo instructions in compliance tests. #106

Closed
jeremybennett opened this issue Apr 22, 2020 · 12 comments
Closed

Use of pseudo instructions in compliance tests. #106

jeremybennett opened this issue Apr 22, 2020 · 12 comments

Comments

@jeremybennett
Copy link
Collaborator

This is a discussion point - I am not clear what the correct answer is.

In issue #105, I note that the regression tests use one of the assembler pseudo instructions, la, in a non-standard way.

        la rd, symbol

is a shorthand for

        auipc rd, symbol[31:12]
        addi rd, rd, symbol[11:0]

This issue is to raise the question of whether the compliance tests should use pseudo instructions at all. RISC-V is perhaps unusual in having quite so many pseudo instructions (I think there are 47 at present). As issue #105 shows, not all are reliably implemented. In part they are a legacy of the attempt to bring up a compiler tool chain very quickly nearly a decade ago.

Pseudo instructions are inherently opaque, and can potentially change (for example choice of insturction for nop), although this seems less likely for RISC-V, since they form part of the standard.

As we discussed in the early days of the compliance standard, having to use an assembler at all is already a compromise. We are testing compliance of implementations of the architecture, not compliance of the assembler, and as we see this is an area where assemblers differ.

I propose that the compliance test suite should use no pseudo instructions and stick strictly to plain assembler opcodes.

@aswaterman
Copy link

This issue is to raise the question of whether the compliance tests should use pseudo instructions at all.

IMO, this is only one step away from suggesting the compliance tests shouldn't rely on the assembler at all, and should instead rewrite its own.

Inconsistencies in the documentation and implementation of pseudoinstructions should be rectified, but that's a bad reason to stop using them.

@jeremybennett
Copy link
Collaborator Author

@aswaterman I agree that the documentation/implementation should be fixed, but that wasn't really the point I was making. It was just what brought it to my attention.

Compliance is (amongst other things), checking that he instructions of the machine behave as the standard says they should. So I think it is better to be explicit about the instructions that are being tested.

@aswaterman
Copy link

@jeremybennett when you're testing auipc, it makes sense to write that instruction explicitly, rather than using la as a proxy for it. When you're testing sfence.vma, you shouldn't feel ashamed to use la as a proxy for auipc.

@simon5656
Copy link
Collaborator

simon5656 commented Apr 22, 2020 via email

@aswaterman
Copy link

aswaterman commented Apr 22, 2020

@simon5656 why use assembler mnemonics at all? Why use a linker? Why not just emit binary code directly?

(The linker remark is not just a flippant comment; it also deletes instructions and replaces some instructions with others, kind of like the thing you're expressing concern over.)

@allenjbaum
Copy link
Collaborator

allenjbaum commented Apr 22, 2020 via email

@aswaterman
Copy link

aswaterman commented Apr 22, 2020 via email

@allenjbaum
Copy link
Collaborator

I actually don't know that how practical it is to remove LA or LI specifically.
IF you know the length of symbol, or of an offset, that's one thing. But compliance tests that load symbols or offsets, and until the loader figures out where things are, you may not know if an offset it 12 bits or 20 bits or 32 bits or 64 bits. To ensure consistancy, you are reduced to always assuming that those constants are 32bits (replacing them with a pair of LUI,ADDI or AUIPC,ADDI) or 64bits (which probably loads from a global constant pool using AUIPC,ADDI,LD triple).
Amusingly, tests for LUI and ADDI already contain those pseudo-ops..

If we were doing that, I'd

  • modify the test spec to prohibit use of LI and LA in the tests (only; not in any other code that gets loaded),
  • define LAX/LIX macros (probably need separate named versions for RV32 and RV64 when we get to tests that can switch between XLEN) that are fixed length, and globally search and replace them in all the tests in the riscv_test_suite subdirectory.

@allenjbaum
Copy link
Collaborator

I think the decision has been made to do precisely what is proposed above (except for the name of the LAX/LIX macros, perhaps). This will be reflected in the next merge of new tests, and a change to the test format spec.
This should be closed when that happens

@jrtc27
Copy link

jrtc27 commented Mar 26, 2021

Do call and tail count as pseudo-instructions? They sure aren't architectural RISC-V instructions, but there's no equivalent expanded form for either of them as they need to be adjacent with a single relocation for the pair in the ELF file, unless you do nasty things with .reloc that I don't think are supported by LLVM, only binutils.

@allenjbaum
Copy link
Collaborator

The prohibition of pseudo instruction in tests is intended to be in just the code in actual .S assembly language tests themselves, and not in supporting code, or directives - if that answers the question. The rationale for this is that if different toolchains generate different expansions, it may mess up how things get loaded between models - and then we've lost PC relative synch points we are depending on. The fear may be an overreaction, but - it wasn't that hard to do.

@allenjbaum
Copy link
Collaborator

The current tests (and not any initialization or trap handling code - just the .S files in the riscv-test-suite/RV32* and /RV64* directories now use constant length LI and LA macros to replace li and la pseudo ops, so closing this one.

topperc pushed a commit to llvm/llvm-project that referenced this issue Jul 3, 2023
This patch improves compatibility with GNU assembler by adding support for
constant immediate in la and lla pseudo instruction, and expanding it in the
same way as we currently expands li pseudo instruction.

Links to discussion related to the above issue in the community -
riscv-non-isa/riscv-arch-test#105
riscv-non-isa/riscv-arch-test#108
riscv-non-isa/riscv-arch-test#106

Reviewed By: craig.topper

Differential Revision: https://reviews.llvm.org/D150133
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

5 participants