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

Allow compressed hints to be expanded #287

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

PeterRugg
Copy link
Contributor

This leads to different values in RVFI. The RISC-V spec allows this behaviour:
"simple implementations can ... execute a HINT as a regular computational instruction".

Without this feature, such implementations cannot be verified using RVFI.

@github-actions
Copy link

github-actions bot commented Jul 19, 2023

Unit Test Results

712 tests  ±0   712 ✔️ ±0   0s ⏱️ ±0s
    6 suites ±0       0 💤 ±0 
    1 files   ±0       0 ±0 

Results for commit 51fd1a7. ± Comparison against base commit ae905fb.

♻️ This comment has been updated with latest results.

c_emulator/riscv_sim.c Outdated Show resolved Hide resolved
This leads to different values in RVFI.
The RISC-V spec allows this behaviour:
"simple implementations can ... execute a HINT as a regular
computational instruction".
Without this feature, such implementations cannot be verified
using RVFI.
@@ -120,6 +120,7 @@ static struct option options[] = {
{"disable-writable-misa", no_argument, 0, 'I'},
{"disable-fdext", no_argument, 0, 'F'},
{"mtval-has-illegal-inst-bits", no_argument, 0, 'i'},
{"c-hints-expand", no_argument, 0, 'H'},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would rather not have 'H' used as the single character switch, as it probably should be reserved for hypervisor. I would suggest to only have a long option.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy to, although if I understand correctly, it will mean some refactoring of this file, as currently all the options have short forms and that's what is selected for in the big switch statement.

Copy link
Contributor

@charmitro charmitro Jul 21, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can use any number beyond the ascii-table. Just remember to remove the short form of this option from the print_usage() function.

For example,

#DEFINE C_HINTS_EXPAND_OPT 1000

{"c-hints-expand",              no_argument,       0, C_HINTS_EXPAND_OPT},

case C_HINTS_EXPAND_OPT:
     ...

Copy link
Collaborator

@billmcspadden-riscv billmcspadden-riscv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Shouldn't the compressed hints always be expanded? In other words, why do we need rv_c_hints_expand?
  2. The expansion seems correct to me.
  3. Testing: what tests exist? Shouldn't we have some tests for checking this functionality as part of the PR?

@PeterRugg
Copy link
Contributor Author

@billmcspadden-riscv

  1. I didn't think I should disrupt the sail model that much: presumably somebody did this intentionally because they had to jump through hoops to do it. I think it's useful that they get labeled as "HINT"s at the very least, so it's clear from the trace if you're running code that's expecting some extension that uses them. Presumably when HINT encodings actually get used, they'll be defined in this file. That said, it suits my use-case for them to be just decoded as normal without special casing, so I wouldn't mind that at all.
  2. Thank you for checking.
  3. You're quite right: I can look into adding tests if there's agreement that this PR might get merged.

@PeterRugg
Copy link
Contributor Author

A few thoughts on testing, as I'm not super clear on what tests would look like:

  1. It seems the HINT cases should be included in the RVC tests regardless of whether this PR goes ahead, as I've heard two implementations now (including early sail?) misread the spec and made them illegal. I guess a test case that runs the HINT and checks that there's no trap would be a start. Of course that would mean that a future extension processor that actually used the HINTs for something would fail the tests, which is presumably why they aren't tested currently?
  2. Asserting "this instruction didn't do anything" seems tricky, and most instruction tests presumably don't bother checking that nothing changed that wasn't supposed to.
  3. As far as I can tell, the tests are all written in terms of architecturally observable behaviour, so this option wouldn't change anything?
  4. Surely we don't want to run all the tests again for both values of the "c-expand-hints" option: is there a way of selecting a subset of tests that run for given option combinations?

@jrtc27
Copy link
Collaborator

jrtc27 commented Jul 21, 2023

The current Sail model hints implementation is a continued mess of people overcomplicating things and doing the wrong thing. See #112 and follow the chain back to earlier discussions. I fully support completely axing the distinction between hints and non-hints at decode time; it’s unnecessary complexity, confusing for people reading the model and gets the disassembly wrong (using some self-invented nonsense mnemonics rather than what both GNU and LLVM toolchains use). The only special casing should be for when extensions are enabled that give the hints additional meaning.

@PeterRugg
Copy link
Contributor Author

@jrtc27 Fine by me. I'm happy to do a PR to simplify things back again.

@jrtc27
Copy link
Collaborator

jrtc27 commented Jul 21, 2023

I honestly don’t know why #31 wasn’t merged and instead the current approach was written and merged. It should just be as I originally proposed.

@PeterRugg
Copy link
Contributor Author

I guess the prose spec really doesn't allow that interpretation: presumably they want to make absolutely sure that non-extended code can't use the HINTs and expect them to be nops.

@jrtc27
Copy link
Collaborator

jrtc27 commented Jul 21, 2023

That’s how toolchains implement them and cores that don’t give the hints special meaning implement them. That’s how the spec intends them to be implemented without further extensions:

image

I think that’s pretty clear that they really are those instructions without additional hint meaning assigning extensions.

@allenjbaum
Copy link
Collaborator

allenjbaum commented Jul 21, 2023 via email

@jrtc27
Copy link
Collaborator

jrtc27 commented Jul 21, 2023

Also note that there are some (new Zce) compressed ops that don't expand to full width ops.

That doesn’t change anything. Those need implementing directly, but these don’t.

@leesum1
Copy link

leesum1 commented Sep 17, 2024

In the cadd test within riscv-arch-tests, the instruction in inst_5

inst_5:
// rs1==x0, rs2==x2, rs1_val == (2**(xlen-1)-1), rs1_val == 2147483647
// opcode: c.add; op1:x0; op2:x2; op1val:0x0; op2val:-0xb503
TEST_CR_OP( c.add, x0, x2, 0, 0x0, -0xb503, x8, 20, x4) 

is considered an invalid instruction. Has there been any progress regarding hint instructions?

@PeterRugg
Copy link
Contributor Author

Hmm, interesting. IIRC, this change should only affect tracing rather than whether instructions are legal or not.

@arichardson
Copy link
Collaborator

In the cadd test within riscv-arch-tests, the instruction in inst_5

inst_5:
// rs1==x0, rs2==x2, rs1_val == (2**(xlen-1)-1), rs1_val == 2147483647
// opcode: c.add; op1:x0; op2:x2; op1val:0x0; op2val:-0xb503
TEST_CR_OP( c.add, x0, x2, 0, 0x0, -0xb503, x8, 20, x4) 

is considered an invalid instruction. Has there been any progress regarding hint instructions?

This is already a HINT?

mapping clause encdec_compressed = C_ADD_HINT(rs2)
      if rs2 != zreg
  <-> 0b100 @ 0b1 @ 0b00000 @ rs2 : regidx @ 0b10
      if rs2 != zreg

function clause execute (C_ADD_HINT(rs2)) = RETIRE_SUCCESS

The PR here just expands it to actually execute the ADD with zero as the destination.

@Timmmm
Copy link
Collaborator

Timmmm commented Oct 8, 2024

I would support closing this PR and just deleting the hints. The only reason I can think to keep them is for nicer disassembly, but that seems marginal and if the disassembly isn't correct at the moment anyway then I don't think it makes sense to keep them.

Any objections?

@PeterRugg
Copy link
Contributor Author

Would work fine for me!

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

Successfully merging this pull request may close these issues.

8 participants