search

riscv-non-isa / riscv-arch-test

https://jira.riscv.org/browse/RVG-141?src=confmacro
Apache License 2.0
502 stars 191 forks source link

The FPU tests do not compile if rvtest_mtrap_routine is defined #362

Open algrobman opened 1 year ago

algrobman commented 1 year ago

tsig_[begin|end]_canary is mentioned twice in the FP tests sources:

see ex:

https://github.com/riscv-non-isa/riscv-arch-test/blob/873d16e748ad60023dcdda3926144957c096e31d/riscv-test-suite/rv32i_m/F/src/fadd_b1-01.S#L5886

ifdef rvtest_mtrap_routine

tsig_begin_canary: CANARY; tsig_begin_canary: CANARY; mtrap_sigptr: .fill 64*(XLEN/32),4,0xdeadbeef tsig_end_canary: CANARY; tsig_end_canary: CANARY;

endif

It make hard to run them outside riscof , please, fix this in next opportunity

allenjbaum commented 1 year ago

Unclear why that wasn't caught in CI - or why it matters whether it is running under riscof or not.

algrobman commented 1 year ago

RISCOF provides this rvtest_mtrap_routine define as part of run command, I guess for only tests, which suppose to have exceptions .

We had set this define in model_test.h and it worked for all integer tests, until we added FP tests ...

allenjbaum commented 1 year ago

I'm not sure that this is something we need to fix. The tests themselves are supposed to set it. Why are you setting it? What breaks if you don't? Going a bit further: the tests for architectural compatibility are intended to be run under the riscof framework. Riscof looks at the test_case macro parameters in the test to decide whether to install the trap handler

If you need to get branding, then you need to run under riscof - so no changes are needed If you don't need to get branding, there is no need to run the ACTs - so no changes are needed I see no rationale for us to make changes in order to make these tests run independently of riscof; A simpler solution is to modify your model_test.h to avoid setting it when you run FP tests. There is no guarantee you won't run into similar problems with test for other extensions, of course.

algrobman commented 1 year ago

too many words of excuses instead of just fixing plain incorrect code ...

algrobman commented 1 year ago

we use these tests in our verification environment which does not have riscof automatization - so we had to patch their sources. I guess others will eventually face the same problem

allenjbaum commented 1 year ago

We are resource constrained and don't have people to fix things we don't need to support. You are (so far) the only person who has had this issue, leading me to believe it is waaay down on our priority list, for the reasons I've given. ACTs are not a verification tool; you should have your own tests for that. They are a compatibility tool, and if you don't understand the difference you should watch the video of the talk Simon Davidmann and I gave at the December summit. Submit a PR and we'll probably accept it, but that PR will have to work in both environments .

On Wed, Jun 21, 2023 at 6:52 AM algrobman @.***> wrote:

we use these tests in our verification environment which does not have riscof automatization - so we had to patch their sources. I guess others will eventually face the same problem

— Reply to this email directly, view it on GitHub https://github.com/riscv-non-isa/riscv-arch-test/issues/362#issuecomment-1600877847, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHPXVJVPTOPWZALI2CECF23XML4BJANCNFSM6AAAAAAZAZ7AF4 . You are receiving this because you commented.Message ID: @.***>