This revision supports --print-supported-extensions,
it prints out all of the extensions and corresponding version supported.
Details
- Reviewers
asb reames MaskRay craig.topper kito-cheng
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
llvm/lib/Support/RISCVISAInfo.cpp | ||
---|---|---|
152 | Question for the community. Should we maintain SupportedExtensions in the order we want printed instead of sorting? |
clang/lib/Driver/Driver.cpp | ||
---|---|---|
4244–4248 | Plz did the similar action like what OPT_print_supported_cpus did instead of just exti 0 And check the triple to make sure only invoke that for riscv | |
llvm/lib/Support/RISCVISAInfo.cpp | ||
151 | I would prefer that print by canonical order *IF* we want to print in some order. And then you can just use RISCVISAInfo::OrderedExtensionMap here. |
llvm/lib/Support/RISCVISAInfo.cpp | ||
---|---|---|
152 | +1 on Kito's suggestion to print by the canonical order. |
llvm/lib/Support/RISCVISAInfo.cpp | ||
---|---|---|
143 | void llvm::RISCVMarchHelp and remove the namespace llvm { |
clang/test/Driver/print-supported-marchs.c | ||
---|---|---|
17 ↗ | (On Diff #506062) | Use -NEXT: whenever applicable so that new extensions can be caught by the test. |
clang/tools/driver/cc1_main.cpp | ||
197 | The check should be added to lib/Driver | |
201 | riscvMarchHelp Use camelCase for new function names. | |
llvm/lib/Support/RISCVISAInfo.cpp | ||
162 | Move '\n' into the "Use -march to specify the target's extension.\n..." string. |
clang/tools/driver/cc1_main.cpp | ||
---|---|---|
197 | RISCV -> RISC-V in user facing messages. |
clang/tools/driver/cc1_main.cpp | ||
---|---|---|
189 | Why do we need to lookup the TargetRegistry? |
Aliasing -march=help to --print-supported-extensions looks weird. I think this patch should remove -march=help.
clang/test/Driver/print-supported-marchs.c | ||
---|---|---|
1 ↗ | (On Diff #508475) | The filename should be fixed. |
Created a GCC feature request: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109349
The subject and summary should be fixed and -march=help references should be removed.
clang/lib/Driver/Driver.cpp | ||
---|---|---|
4247 | The convention is to use Diag(...). | |
4251 | Why is the code needed? Can --print-supported-extensions reuse the approach of OPT_print_supported_cpus? |
I would support have -march=help, but I know it's really not make scene to alias -march=help to --print-supported-extensions for other targets, what about redirect that in Driver.cpp for RISC-V only?
clang/lib/Driver/Driver.cpp | ||
---|---|---|
4251 | Yes, this should reuse some code with the OPT_print_supported_cpus handler. |
Resolved MaskRay's comments.
Should we use @kito-cheng's approach to deal with -march=help? what do you think, @MaskRay ?
I hope that we add just one option, instead of aliases. Aliases are usually added for compatibility when something is deprecated or changed, not for new options.
My "Request Changes" status still holds. The patch hasn't fixed something obvious: the description mentions print-supported-marchs (well, it should really name the exact name, --print-supported-marchs, don't omit dashes) while the implementation uses ...extensions.
Rename clang/test/Driver/print-supported-marchs.c to clang/test/Driver/print-supported-extensions.c
clang/tools/driver/cc1_main.cpp | ||
---|---|---|
187–188 | Plz make sure only RISC-V print that, x86 or other target print RISC-V's ext is really weird. |
LGTM, consider about the GNU compatibility, I would that has -march=help form for that.
-march= -mcpu= -mtune= are from GCC and they traditionally have many opinions on the features.
If GCC is going to have -march=help, I think adding -march=help to Clang to match is fine.
But I'd really like to avoid aliases for non-compatibility reasons. They just cause confusion and make users undecided about which one is canonical.
Done. But this update still doesn't look quite polished. If -march=help is dropped, at the very least the subject needs to updated.
clang/tools/driver/cc1_main.cpp | ||
---|---|---|
187 | Use camelCase for new function names. StringRef TargetStr? | |
llvm/include/llvm/Support/RISCVISAInfo.h | ||
26 | This naming does not match the --print-supported-extensions option name. | |
llvm/lib/Support/RISCVISAInfo.cpp | ||
143 | For most --print-* options, the output goes to stdout instead of stderr. |
Please also mark resolved comments as done, otherwise reviewers can presume that some comments are unresolved and don't necessarily give another round of reviews.
clang/test/Driver/print-supported-extensions.c | ||
---|---|---|
6 | Use --implicit-check-not=warning: instead of // CHECK-NOT: warning: argument unused during compilation. We additionally check that there is no warning and the warning doesn't come after the output from --print-supported-extensions. | |
6 | For a file with just one prefix, conventionally we just use the default CHECK and omit --check-prefix. Have you ever checked that the test detects issues? CHECK below are ignored with --check-prefix=CHECK-RISCV. |
clang/test/Driver/print-supported-extensions.c | ||
---|---|---|
1 | Some newer tests use /// to distinguish non-CHECK-non-RUN comments from others. This helps automation tools to catch wrong prefixes (e.g. with a typo) in comments and helps auto-generating checks in the future while retaining manual comments. | |
3 | If you remove all and RISCV from LLVM_TARGETS_TO_BUILD, you can test whether this feature works without lib/Target/RISCV support. I suspect it works, so you may need another build for verification. | |
10 | The formatting matters here. To test formatting see --strict-whitespace --match-full-lines (example: See llvm/test/tools/llvm-readobj/ELF/gnu-sections.txt): https://llvm.org/docs/CommandGuide/FileCheck.html |