This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add B extension tests to make sure RV64 only instructions aren't accepted in RV32.
ClosedPublic

Authored by craig.topper on Jan 21 2021, 10:16 AM.

Details

Summary

Add tests to make sure common instructions are accepted in RV64
and not just RV32.

Diff Detail

Event Timeline

craig.topper created this revision.Jan 21 2021, 10:16 AM
craig.topper requested review of this revision.Jan 21 2021, 10:16 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 21 2021, 10:16 AM
Herald added a subscriber: MaskRay. · View Herald Transcript
craig.topper added inline comments.Jan 21 2021, 10:21 AM
llvm/test/MC/RISCV/rv64zbc-valid.s
8–9

This file is new but git and/or arcanist decided to mark it as a copy with changes from a different file so this diff looks funny.

This revision is now accepted and ready to land.Jan 22 2021, 6:54 AM
asb accepted this revision.Jan 22 2021, 10:58 AM

Thanks for extending the test coverage!

Given that other than RVC, the RV64 variants of instruction set extensions are always additive, I have a slight preference for just adding some RV64 RUN lines to the rv32*-valid.s tests rather than copying and pasting the common instructions to the rv64*-valid.s files. This is consistent with what we do for the base extensions.

But this works fine, so I'm happy enough for it to land as is.

In D95150#2516111, @asb wrote:

Thanks for extending the test coverage!

Given that other than RVC, the RV64 variants of instruction set extensions are always additive, I have a slight preference for just adding some RV64 RUN lines to the rv32*-valid.s tests rather than copying and pasting the common instructions to the rv64*-valid.s files. This is consistent with what we do for the base extensions.

But this works fine, so I'm happy enough for it to land as is.

Reducing test duplication would be good, but if B already has a bunch of that then maybe it's better to land this and then have a follow-up commit that deduplicates a la RVI etc.

In D95150#2516111, @asb wrote:

Thanks for extending the test coverage!

Given that other than RVC, the RV64 variants of instruction set extensions are always additive, I have a slight preference for just adding some RV64 RUN lines to the rv32*-valid.s tests rather than copying and pasting the common instructions to the rv64*-valid.s files. This is consistent with what we do for the base extensions.

But this works fine, so I'm happy enough for it to land as is.

zext.h and rev8 from Zbb have different encodings between rv32 and rv64. Should we use different check prefixes for those?

In D95150#2516111, @asb wrote:

Thanks for extending the test coverage!

Given that other than RVC, the RV64 variants of instruction set extensions are always additive, I have a slight preference for just adding some RV64 RUN lines to the rv32*-valid.s tests rather than copying and pasting the common instructions to the rv64*-valid.s files. This is consistent with what we do for the base extensions.

But this works fine, so I'm happy enough for it to land as is.

zext.h and rev8 from Zbb have different encodings between rv32 and rv64. Should we use different check prefixes for those?

For RVC we have an rv32c-only-valid.s to avoid the need to mess with check prefixes.

This revision was landed with ongoing or failed builds.Jan 22 2021, 1:52 PM
This revision was automatically updated to reflect the committed changes.