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

Correcting IO assertion macros #426

Merged
merged 2 commits into from
Jun 3, 2024
Merged

Conversation

dpetrisko
Copy link
Contributor

Description

This PR corrects the IO assertion macros in TEST_CASE_F and TEST_CASE_FID. Currently, they call GPR comparison, but these are floating point test cases. This causes predictable errors when compiling with any reasonable implementation of RVMODEL_IO_ASSERT_GPR_EQ.

riscv-arch-test/riscv-test-suite/rv64i_m/D/src/fcvt.d.lu_b26-01.S:42: Error: illegal operands `ld t1,0x08(f20)'
...
TEST_FPIO_OP(fcvt.d.lu, f20, x19, dyn, 0, 0, x9, 0*8, x11, x1, x5,ld)
...
#define TEST_FPIO_OP( inst, destreg, freg, rm, fcsr_val, correctval, valaddr_reg, val_offset, flagreg, swreg, testreg, load_instr) \
    TEST_CASE_F(testreg, destreg, correctval, swreg, flagreg,        \
      LOAD_MEM_VAL(load_instr, valaddr_reg, freg, val_offset, testreg)  ;\
      li testreg, fcsr_val; csrw fcsr, testreg  ;\
      inst destreg, freg, rm            ;\
      csrr flagreg, fcsr            ;\
    )

Related Issues

NA

Ratified/Unratified Extensions

  • Ratified
  • Unratified

List Extensions

F,D

Reference Model Used

  • SAIL
  • Spike
  • Other - < SPECIFY HERE >

Mandatory Checklist:

  • All tests are compliant with the test-format spec present in this repo ?
  • Ran the new tests on RISCOF with SAIL/Spike as reference model successfully ?
  • Ran the new tests on RISCOF in coverage mode
  • Link to Google-Drive folder containing the new coverage reports (See this for more info): < SPECIFY HERE >
  • Link to PR in RISCV-ISAC from which the reports were generated : < SPECIFY HERE >
  • Changelog entry created with a minor patch

Optional Checklist:

  • RISCV-V CTG PR link if tests were generated using it : < SPECIFY HERE >
  • Were the tests hand-written/modified ?
  • Have you run these on any hard DUT model ? Please specify name and provide link if possible in the description
  • If you have modified arch_test.h Please provide a detailed description of the changes in the Description section above.

@dpetrisko
Copy link
Contributor Author

Not sure on the checklists here. Let me know if you need help from me on it, but it's a fairly trivial change that should not break anything™

Copy link
Collaborator

@allenjbaum allenjbaum left a comment

Choose a reason for hiding this comment

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

I'm a bit puzzled as to what is going on here, because I don't see the code you're fixing in the repo. The current version I see doesn't use RVMODEL_IO_ASSERT_SFPR_EQ, it uses RVMODEL_IO_ASSERT_GPR_EQ - which is wrong, but it isn't what this PR is updating. What is the source of this?
The current version of the repo already has this fix.

The correct fix for the first change would be to change it to be a model defined RVMODEL_IO_ASSERT_FPR_EQ, which should use the FLREG/FSREG macros that take into account the current FLEN, just as your fix (and other macros) does.

@jamesbeyond jamesbeyond added the bug-fix a PR with relevant small changes or fixed a bug label May 13, 2024
@jamesbeyond jamesbeyond changed the base branch from main to dev May 28, 2024 16:55
dpetrisko added 2 commits June 4, 2024 00:30
Signed-off-by: Dan Petrisko <[email protected]>
Copy link
Collaborator

@allenjbaum allenjbaum left a comment

Choose a reason for hiding this comment

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

This is correctr now

Copy link
Collaborator

@allenjbaum allenjbaum left a comment

Choose a reason for hiding this comment

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

This is correct now

@allenjbaum allenjbaum merged commit ff31610 into riscv-non-isa:dev Jun 3, 2024
@dpetrisko
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-fix a PR with relevant small changes or fixed a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants