search

riscv-software-src / riscof

BSD 3-Clause "New" or "Revised" License
61 stars 39 forks source link

Mechanism needed to append to test RVTEST_ISA string #35

Open nadiga-ventana opened 2 years ago

nadiga-ventana commented 2 years ago

All riscv tests have RVTEST_ISA macro fro which the isa string is extracted to be passed as -march option to the compiler. The rv64i_m/C/src/cebreak-01.S has RVTEST_ISA("RV64IC") but it also defines rvtest_mtrap_routine which includes CSRRW instruction. Our new compiler (riscv64-unknown-elf-gcc) needs the -march option to have Zicsr to recognize these csrrw instructions. Is there a way to append to the RVTEST_ISA string ?

pawks commented 2 years ago

You will need to add _Zicsr in the argument to RVTEST_ISA for all the tests which need this extension. You will also need to modify the plugin to include the extension in the output march argument if it is present in the string. For the test rv64i_m/C/src/cebreak-01.S the macro will look like this - RVTEST_ISA("RV64IC_Zicsr")

nadiga-ventana commented 2 years ago

Instead of changing the argument in the RVTEST_ISA macro for each of the test, can the riscof setup not use the ISA string the plugin of the DUT for generating the march argument for the compiler?

This would also be required if the test does not use rvtest_mtrap_routine , but has the RVMODEL_BOOT modified to use CSRRW or other instructions that require extensions that are not part RVTEST_ISA string.

allenjbaum commented 2 years ago

I had the same thought. The tests have no visibility as to whether any of the RVMODEL_* macros use CSRs or not - not just RVMODEL_BOOT. It wouldn't be surprising if RVMODEL_HALT did, for example. Therefore we either need to have all tests default to Zicsr, or require the vendors to explicitly set it ...somewhere... if they use CSRs in their support macros. Their config YAML should have this information

pdonahue-ventana commented 2 years ago

Therefore we either need to have all tests default to Zicsr, or require the vendors to explicitly set it ...somewhere... if they use CSRs in their support macros. Their config YAML should have this information

I agree that the -march string should be based on the config YAML instead of RVTEST_ISA. RVTESTISA should only affect whether or not the test runs at all. The full list of extensions listed in the YAML ought to be available to RVMODEL* macros so somebody can, for instance, initialize whatever state they need to initialize even if that state uses extensions that are outside of the minimal set used by the test itself.

pawks commented 2 years ago

The objective of the RVTEST_ISA macro is to convey the minimal list of extensions needed for the test to be compiled. This is to serve as a hint for the plugins to derive the march string. If any extra extensions are needed for any of the RV_MODEL* macros, the plugin is free to add them to the march string(care should be taken that any conflicting extensions are not included, as far as the current tests go the C extension shouldn't be add unless specified). The vendors have control over how the march is derived from the ISA mentioned in the test. The selection of the test is not dependent on the RVTEST_ISA macro, it is dependent on the conditions listed in the RVTEST_CASE macro.

allenjbaum commented 2 years ago

OK, then we just need to document very clearly (but mostly very prominently) somewhere that Zicsr must be added to the march string (if not already added ) if RVMODEL_ macros have any CSR reads or writes. I'm sure we will get people filing issues because they missed that bit of information.

On Wed, Feb 23, 2022 at 11:38 AM S Pawan Kumar @.***> wrote:

The objective of the RVTEST_ISA macro is to convey the minimal list of extensions needed for the test to be compiled. This is to serve as a hint for the plugins to derive the march string. If any extra extensions are needed for any of the RV_MODEL* macros, the plugin is free to add them to the march string(care should be taken that any conflicting extensions are not included, as far as the current tests go the C extension shouldn't be add unless specified). The vendors have control over how the march is derived from the ISA mentioned in the test. The selection of the test is not dependent on the RVTEST_ISA macro, it is dependent on the conditions listed in the RVTEST_CASE macro.

— Reply to this email directly, view it on GitHub https://github.com/riscv-software-src/riscof/issues/35#issuecomment-1049143562, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHPXVJS6WOUGSW5FHAG6VVLU4UZSNANCNFSM5PESMGMQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

You are receiving this because you commented.Message ID: @.***>