Page MenuHomePhabricator

[RISCV] Verify the input value of -march=
ClosedPublic

Authored by kito-cheng on Mar 6 2018, 10:24 PM.

Details

Summary

This patch doing more check and verify the -march= string and will issue an error if it's a invalid combination.

Diff Detail

Repository
rC Clang

Event Timeline

kito-cheng created this revision.Mar 6 2018, 10:24 PM
apazos added inline comments.Mar 7 2018, 1:46 PM
lib/Driver/ToolChains/Arch/RISCV.cpp
49

In the switch cases move default to first position.

88

So the subsequent features can appear in any order?

kito-cheng added a subscriber: kito-cheng.
kito-cheng marked 2 inline comments as done.Mar 7 2018, 11:40 PM
kito-cheng added inline comments.
lib/Driver/ToolChains/Arch/RISCV.cpp
49

Done :)

88

Yeah, here is a canonical order specified in ISA manual, I've check the order now.

kito-cheng marked 4 inline comments as done.Mar 7 2018, 11:40 PM
kito-cheng updated this revision to Diff 137687.Mar 8 2018, 6:50 PM

This version only update variable name which changed in last version by accident.

kito-cheng edited the summary of this revision. (Show Details)Mar 9 2018, 12:53 AM

Add test cases for the correct inputs.

asb added a comment.Mar 14 2018, 1:34 PM

Thanks for submitting this Kito. I've added some minor in-line comments. It might also be worth adding a couple of extra cases to the tests:

  • Repeated letters in the ISA string (e.g. rv32immafd)
  • Upper case letters in the ISA string. We currently reject these (as does GCC). It would be worth having a test that tracks this behaviour

We could probably give more informative error messages than just "invalid arch name". Especially for common errors like -march=rv32. If not adding such diagnostics in this patch, it would be good to add a // TODO note to record that we'd like to do better.

lib/Driver/ToolChains/Arch/RISCV.cpp
35

I'd reference Table 22.1 in RISC-V User-Level ISA V2.2 for anyone who wants to verify this.

38–39

Should be HasF, HasD, and Baseline to conform to standard LLVM naming conventions.

40

I'd phrase this as "Check ISA extensions are specified in the canonical order."

107

I'd be tempted to give a bit more explanation a bit more "It's illegal to specify the 'd' (double-precision floating point) extension without also specifying the 'f' (single precision floating-point) extension".

Update revision according Alex's review.

Changes:

  • Add testcase for uppercase of -march string.
  • Add testcase for repeated letter in -march.
  • Add more comment.
  • Add several TODO item for diagnostic message improvement.
  • Fix coding style issue.
kito-cheng marked 4 inline comments as done.Mar 21 2018, 12:57 AM

Hi Alex:

Thanks for your input, check for repeated letter was missed in my last patch :)

lib/Driver/ToolChains/Arch/RISCV.cpp
35

Done.

38–39

Fixed.

40

Done.

107

Done.

kito-cheng marked 8 inline comments as done.Mar 21 2018, 12:57 AM
asb accepted this revision.Mar 22 2018, 4:19 AM

Thanks for this Kito. A tiny formatting nit, but otherwise this looks good to me.

lib/Driver/ToolChains/Arch/RISCV.cpp
38

Indent should be 2 spaces rather than 3.

This revision is now accepted and ready to land.Mar 22 2018, 4:19 AM
apazos added inline comments.Mar 27 2018, 10:25 AM
lib/Driver/ToolChains/Arch/RISCV.cpp
51

One more question - how about non-standard extensions (vendor/custom) prefixed with X?
Shouldn't we add the logic to process 'Xext' occurrences in the march string as part of this patch?

This revision was automatically updated to reflect the committed changes.