[RISCV] Add --print-supported-extensions support

[4vtomat]

Summary

This revision supports --print-supported-extensions,
it prints out all of the extensions and corresponding version supported.

[craig.topper]

clang/lib/Driver/Driver.cpp

110

Can we declare this in RISCVISAInfo.h and include that here?

llvm/lib/Support/RISCVISAInfo.cpp

142

This function is not in the llvm namespace or a namespace nested under it.

[craig.topper]

llvm/lib/Support/RISCVISAInfo.cpp

152

Question for the community. Should we maintain SupportedExtensions in the order we want printed instead of sorting?

[kito-cheng]

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.

[asb]

llvm/lib/Support/RISCVISAInfo.cpp

152

+1 on Kito's suggestion to print by the canonical order.

[4vtomat]

Resolved Craig and Kito's comments.

[craig.topper]

llvm/lib/Support/RISCVISAInfo.cpp

143

void llvm::RISCVMarchHelp and remove the namespace llvm {

[MaskRay]

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.

[craig.topper]

clang/tools/driver/cc1_main.cpp

197

RISCV -> RISC-V in user facing messages.

[4vtomat]

Resolved MaskRay and Craig's comments.

[craig.topper]

clang/tools/driver/cc1_main.cpp

189

Why do we need to lookup the TargetRegistry?

[craig.topper]

clang/include/clang/Driver/Options.td

4346

Maybe this should be -print-supported-extensions to match RISC-V terminology?

4349

There's no default target behavior. It's always RISC-V.

clang/lib/Driver/Driver.cpp

4247

"-march=help is only supported for RISC-V"

[4vtomat]

Resolved Craig's comments.

[craig.topper]

LGTM

[MaskRay]

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.

[MaskRay]

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?

[kito-cheng]

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?

[craig.topper]

clang/lib/Driver/Driver.cpp

4251

@MaskRay are you just asking to use the same block of code for both cases?

[MaskRay]

clang/lib/Driver/Driver.cpp

4251

Yes, this should reuse some code with the OPT_print_supported_cpus handler.

[4vtomat]

Resolved MaskRay's comments.
Should we use @kito-cheng's approach to deal with -march=help? what do you think, @MaskRay ?

[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.

[4vtomat]

Rename clang/test/Driver/print-supported-marchs.c to clang/test/Driver/print-supported-extensions.c

[kito-cheng]

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.

[4vtomat]

clang/lib/Driver/Driver.cpp

110

Yes, we can.

clang/tools/driver/cc1_main.cpp

187–188

The check is in clang/lib/Driver/Driver.cpp:4225~

189

I forgot to remove lol~

[kito-cheng]

LGTM, consider about the GNU compatibility, I would that has -march=help form for that.

[craig.topper]

In D146054#4314766, @kito-cheng wrote:

LGTM, consider about the GNU compatibility, I would that has -march=help form for that.

Are you saying gcc supports march=help? Is that recent?

[MaskRay]

-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.

[4vtomat]

Remove -march=help alias.

[kito-cheng]

GCC ins't implement yet, but planed, so add it later I think?

@4vtomat already drop -march=help, @MaskRay did you mind take a look again?

[MaskRay]

In D146054#4337866, @kito-cheng wrote:

GCC ins't implement yet, but planed, so add it later I think?

@4vtomat already drop -march=help, @MaskRay did you mind take a look again?

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.

[4vtomat]

Resolved MaskRay's comments.

[MaskRay]

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.

[MaskRay]

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