Page MenuHomePhabricator

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

Authored by 4vtomat on Mar 14 2023, 8:17 AM.

Details

Summary

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

Diff Detail

Event Timeline

4vtomat created this revision.Mar 14 2023, 8:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2023, 8:17 AM
4vtomat requested review of this revision.Mar 14 2023, 8:17 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMar 14 2023, 8:17 AM
4vtomat edited the summary of this revision. (Show Details)Mar 14 2023, 8:20 AM
craig.topper added inline comments.
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 added inline comments.Mar 15 2023, 6:30 PM
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 added inline comments.Mar 16 2023, 12:18 AM
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 added inline comments.Mar 16 2023, 11:59 AM
llvm/lib/Support/RISCVISAInfo.cpp
152

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

4vtomat updated this revision to Diff 506062.Mar 17 2023, 6:19 AM

Resolved Craig and Kito's comments.

craig.topper added inline comments.Mar 20 2023, 10:27 AM
llvm/lib/Support/RISCVISAInfo.cpp
143

void llvm::RISCVMarchHelp and remove the namespace llvm {

MaskRay requested changes to this revision.Mar 20 2023, 10:28 AM
MaskRay added inline comments.
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.

This revision now requires changes to proceed.Mar 20 2023, 10:28 AM
craig.topper added inline comments.Mar 20 2023, 10:33 AM
clang/tools/driver/cc1_main.cpp
197

RISCV -> RISC-V in user facing messages.

4vtomat updated this revision to Diff 508461.Mar 26 2023, 7:54 PM

Resolved MaskRay and Craig's comments.

craig.topper added inline comments.Mar 26 2023, 8:23 PM
clang/tools/driver/cc1_main.cpp
189

Why do we need to lookup the TargetRegistry?

craig.topper added inline comments.Mar 26 2023, 8:27 PM
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 updated this revision to Diff 508475.Mar 26 2023, 9:41 PM

Resolved Craig's comments.

MaskRay requested changes to this revision.Mar 30 2023, 9:34 PM

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.

This revision now requires changes to proceed.Mar 30 2023, 9:34 PM

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?

craig.topper added inline comments.Apr 7 2023, 12:56 PM
clang/lib/Driver/Driver.cpp
4251

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

MaskRay added inline comments.Apr 11 2023, 5:31 PM
clang/lib/Driver/Driver.cpp
4251

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

4vtomat updated this revision to Diff 514302.EditedApr 17 2023, 10:36 AM

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.

4vtomat retitled this revision from [RISCV] Add -print-supported-marchs and -march=help support to [RISCV] Add --print-supported-extensions and -march=help support.Apr 23 2023, 2:10 AM
4vtomat edited the summary of this revision. (Show Details)
4vtomat updated this revision to Diff 516708.Apr 25 2023, 1:49 AM

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

kito-cheng added inline comments.Apr 28 2023, 5:10 AM
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 added inline comments.Tue, May 2, 5:13 PM
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 accepted this revision.Wed, May 3, 12:38 AM

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

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?

-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 updated this revision to Diff 520904.Tue, May 9, 9:38 PM

Remove -march=help alias.

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?

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 updated this revision to Diff 522411.Mon, May 15, 7:49 PM

Resolved MaskRay's comments.

4vtomat retitled this revision from [RISCV] Add --print-supported-extensions and -march=help support to [RISCV] Add --print-supported-extensions support.Mon, May 15, 7:49 PM
4vtomat edited the summary of this revision. (Show Details)
evandro removed a subscriber: evandro.Wed, May 17, 3:56 PM

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 requested changes to this revision.Thu, May 18, 11:02 AM
MaskRay added inline comments.
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

This revision now requires changes to proceed.Thu, May 18, 11:02 AM