Page MenuHomePhabricator

[clang][RISCV][test] Add more tests of the -mabi and -march options
ClosedPublic

Authored by benshi001 on Jun 8 2021, 2:43 AM.

Details

Summary
  1. There is no tests for mabi=ilp32e, and my patch covers that.
  2. The tests in riscv-abi.c will show default ABI changes for special archs in the future, especially the arch with the F but without the D extension.
  3. The tests in riscv-arch.c will show default arch changes for abi=ilp32, which is rv32imacfd currently, but it is better to be rv32imac. And it is also better for abi=ilp32f defaults to arch=imacf.

Diff Detail

Event Timeline

benshi001 created this revision.Jun 8 2021, 2:43 AM
benshi001 requested review of this revision.Jun 8 2021, 2:43 AM
benshi001 retitled this revision from [RISCV][clang] to [RISCV][clang] Improve deduction of default ABI for some special archs.Jun 8 2021, 2:48 AM
benshi001 edited the summary of this revision. (Show Details)
benshi001 set the repository for this revision to rG LLVM Github Monorepo.
Herald added a project: Restricted Project. · View Herald TranscriptJun 8 2021, 2:48 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
benshi001 updated this revision to Diff 350755.Jun 8 2021, 6:29 PM
benshi001 retitled this revision from [RISCV][clang] Improve deduction of default ABI for some special archs to [clang][RISCV][test] Add more tests of the -mabi and -march options.
benshi001 edited the summary of this revision. (Show Details)

How to formulate the default abi and arch is still under discussion.

https://github.com/riscv/riscv-toolchain-conventions/issues/13

But the tests can be added first, for contrast of future changes.

How do the new tests provide additional coverage?

clang/test/Driver/riscv-arch.c
45

I suggest the style used in linux-cross.cpp

Just using -SAME cannot detect unrelated strings between "-target-feature" "+m" and "-target-feature" "+f"

benshi001 updated this revision to Diff 350788.Jun 8 2021, 11:23 PM
benshi001 marked an inline comment as done.
benshi001 added inline comments.
clang/test/Driver/riscv-arch.c
45

Thanks for your suggestion.

benshi001 marked an inline comment as done.Jun 8 2021, 11:24 PM

How do the new tests provide additional coverage?

For these tests,

  1. there is no tests for mabi=ilp32e, and my patch covers that.
  2. the tests in riscv-abi.c will show default abi changes for special archs, especially for the arch with F but without D extension, in the future.
  3. the tests in riscv-arch.c will show default arch changes for abi=ilp32, which is rv32imacfd now, and it is better to be rv32imac. for abi=ilp32f it is better arch=imacf than current imacfd.
benshi001 updated this revision to Diff 350828.Jun 9 2021, 2:31 AM
  1. there is no tests for mabi=ilp32e, and my patch covers that.
  2. the tests in riscv-abi.c will show default abi changes for special archs, especially for the arch with F but without D extension, in the future.
  3. the tests in riscv-arch.c will show default arch changes for abi=ilp32, which is rv32imacfd now, and it is better to be rv32imac. for abi=ilp32f it is better arch=imacf than current imacfd.

That sounds like a good description to add to the patch summary / commit message!

benshi001 edited the summary of this revision. (Show Details)Jun 9 2021, 4:51 PM
  1. there is no tests for mabi=ilp32e, and my patch covers that.
  2. the tests in riscv-abi.c will show default abi changes for special archs, especially for the arch with F but without D extension, in the future.
  3. the tests in riscv-arch.c will show default arch changes for abi=ilp32, which is rv32imacfd now, and it is better to be rv32imac. for abi=ilp32f it is better arch=imacf than current imacfd.

That sounds like a good description to add to the patch summary / commit message!

Done. Thanks.

MaskRay accepted this revision.Jun 9 2021, 5:00 PM

LGTM.

clang/test/Driver/riscv-arch.c
122
// CHECK-LP64F:      "-target-feature" "+m"
// CHECK-LP64F-SAME: {{^}} "-target-feature" "+a"
This revision is now accepted and ready to land.Jun 9 2021, 5:00 PM
benshi001 marked an inline comment as done.Jun 9 2021, 6:15 PM