This is an archive of the discontinued LLVM Phabricator instance.

[Driver] Add -print-multi-flags-experimental option
ClosedPublic

Authored by michaelplatings on Jan 30 2023, 1:52 PM.

Details

Summary

This option causes the flags used for selecting multilibs to be printed.
This is an experimental feature that is documented in detail in D143587.

Diff Detail

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptJan 30 2023, 1:52 PM
michaelplatings requested review of this revision.Jan 30 2023, 1:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 30 2023, 1:52 PM
Joe added a subscriber: Joe.Feb 1 2023, 9:09 AM

So again, I took these patches for a spin with RISC-V. As I mentioned before, I was using a GCC installation, so the multilib selection happens inside Gnu.cpp (findRISCVBareMetalMultilibs). The main trouble I had was at that point we don't have a reference to the ToolChain, so calling getMultiSelectionFlags was not possible. I had a go at making this function static, and it was mostly fine as you could just pass in the Triple and Driver, however getSanitizerArgs was not possible. I'm not sure what the solution here is.

Multilibs for RISCV are straightforward, they just depend arch+abi, so I added the arch extensions and abi to the Result in getMultiSelectionFlags.

With that modified getMultiSelectionFlags, it worked well :)

The main trouble I had was at that point we don't have a reference to the ToolChain, so calling getMultiSelectionFlags was not possible. [...] I'm not sure what the solution here is.

I see two options:

  1. Split getMultiSelectionFlags into needs-ToolChain and doesn't-need-ToolChain parts, with the former calling the latter. From Gnu.cpp, call the latter, with the limitation that it can use fewer flags.
  2. Pass a ToolChain to the Gnu.cpp functions.

I added the arch extensions and abi to the Result in getMultiSelectionFlags

What was the syntax you used for that?
I'm not super keen on the march=+ext syntax I came up with so I'm open to alternatives.

Joe added a comment.Feb 2 2023, 2:23 AM

The main trouble I had was at that point we don't have a reference to the ToolChain, so calling getMultiSelectionFlags was not possible. [...] I'm not sure what the solution here is.

I see two options:

  1. Split getMultiSelectionFlags into needs-ToolChain and doesn't-need-ToolChain parts, with the former calling the latter. From Gnu.cpp, call the latter, with the limitation that it can use fewer flags.
  2. Pass a ToolChain to the Gnu.cpp functions.

I did briefly try to pass a ToolChain to Gnu.cpp functions, but it became intrusive and had to add it in 5+ places, so unless I'm missing a trick to get the ToolChain from the Driver, my vote would go to the former.

I added the arch extensions and abi to the Result in getMultiSelectionFlags

What was the syntax you used for that?
I'm not super keen on the march=+ext syntax I came up with so I'm open to alternatives.

I dropped the march= and used the same format as target-features, so it was simply +m or +a. I think this is intuitive enough without the march=, and the form x=y is already broken when you add the flags from the flag list. I don't have a strong stance on this though.

I did briefly try to pass a ToolChain to Gnu.cpp functions, but it became intrusive and had to add it in 5+ places, so unless I'm missing a trick to get the ToolChain from the Driver, my vote would go to the former.

The Driver class does in fact have a private getToolChain method. So two new possibilities:

  1. Make getToolChain public.
  2. Make getMultiSelectionFlags a method of Driver.

I dropped the march= and used the same format as target-features, so it was simply +m or +a. I think this is intuitive enough without the march=

Thanks for explaining. I found that when I was writing a regex to match flags it was helpful to have a part of the string I could be sure would be there to avoid matching the wrong type of flag, and march= helped with that. I also worry that names from different types of flags could clash without some kind of namespacing.

the form x=y is already broken when you add the flags from the flag list.

Can you give an example of what you mean by that? Sounds like something that might need fixing.

Joe added a comment.Feb 2 2023, 6:15 AM

Thanks for explaining. I found that when I was writing a regex to match flags it was helpful to have a part of the string I could be sure would be there to avoid matching the wrong type of flag, and march= helped with that. I also worry that names from different types of flags could clash without some kind of namespacing.

Understandable about the flag names clashing. The scope of riscv multilibs was just arch/abi, so there was no concern about clashing/matching the wrong type of flag. Would it be weird for one target to have the march= but anothers not?

clang/lib/Driver/Driver.cpp
2221

Do we want to parse the multilib.yaml here so we can print out custom flags as well? It could help diagnose issues people have with them.

clang/lib/Driver/ToolChain.cpp
206

the form x=y is already broken when you add the flags from the flag list.

Can you give an example of what you mean by that? Sounds like something that might need fixing.

For example the option name for OPT_fexceptions is just fexceptions, and this is added directly to Results

michaelplatings marked an inline comment as done.Feb 2 2023, 7:27 AM

Would it be weird for one target to have the march= but anothers not?

Yes I think it would be weird. Potentially you could have a toolchain supporting Arm and other architectures so it would be unfortunate for that to be inconsistent. I'd very much like the API to work as well as possible across all architectures now, because it's going to be hard to change later. It's great getting your feedback on this.

clang/lib/Driver/Driver.cpp
2221

Yes, I think that would be an improvement. Might need to go in a later patch though.

clang/lib/Driver/ToolChain.cpp
206

OK, I was thrown by the word "broken".
The syntax for the multilib flags is derived from the command line arguments so I think this is fine.

I did briefly try to pass a ToolChain to Gnu.cpp functions, but it became intrusive and had to add it in 5+ places, so unless I'm missing a trick to get the ToolChain from the Driver, my vote would go to the former.

I looked into moving getMultiSelectionFlags into the Driver class but it felt wrong.

I think the place you need to call getMultiSelectionFlags is from findRISCVBareMetalMultilibs and findRISCVMultilibs. They will indeed need changing to take a const ToolChain&. They are called from ScanGCCForMultilibs which is a method of GCCInstallationDetector. GCCInstallationDetector could have a const ToolChain& member. GCCInstallationDetector is only instantiated from the Generic_GCC constructor - Generic_GCC is a subclass of ToolChain so it can pass *this to the GCCInstallationDetector constructor. I think that change would not be too intrusive.

Put "experimental" in the argument name. The intention is to include the new multilib as an experimental feature in LLVM 17 and stabilise it in LLVM 18.

Also remove support for -f(no-)experimental-relative-c++-abi-vtables from the new multilib since Fuchsia no longer needs it.

Print flags expanded by any regular expression matchers in multilib.yaml

michaelplatings marked an inline comment as done.Feb 2 2023, 1:03 PM
michaelplatings added inline comments.
clang/lib/Driver/Driver.cpp
2221

Should be working now.

Bugfix: if -msoft-float is set then -mfpu should be none.

peter.smith added inline comments.
clang/include/clang/Driver/Options.td
4333

any particular reason for using multi rather than multilib? Is there any intent to use this anywhere else?

clang/include/clang/Driver/ToolChain.h
289

Could be worth using "The command line arguments ..." when I first read I didn't pick up on the distinction between flags and arguments. IIUC this function translates command line arguments into normalised multilib selection flags.

303

typo "a pseudo"

clang/lib/Driver/ToolChain.cpp
176–271

IIUC this needs to contain the union of all command-line options used by all clients of the YAML but not any of the existing hard-coded Multilibs?

There is a possibility that this function could get large over time, with many Toolchain and Architecture specific flags? It is it worth delegating some of the work to the Toolchain, for example at the moment the BareMetal driver won't support the sanitizers so this could be processed by a Toolchain specific function (Fuchsia in this case?). There's likely to be a lot of options used by many Multilib implementations so there is still scope for putting stuff here. We would need to know what driver and architecture we are using here though.

If we had already constructed the MultilibSet it could be possible to filter the flags against what the MultilibSet actually accepts?

Is it worth making that clear, or have some way for the other non YAML users to call print_multi_selection_flags? Or is the option purely for developing/debugging multilib as I'd not expect the output of print_multi_section flags to be used by a user of clang. In that case a translation of what command-line flags influence multilib selection (ideally filtered) through the processed YAML file would be great.

223

Even though it might still live in this file, might be worth having all options for a particular arch in a separate function. For example:

case llvm::Triple::arm:
case llvm::Triple::armeb:
case llvm::Triple::thumb:
case llvm::Triple::thumbeb:
  addArmArchFlags(); // not put any thought into parameters etc.
228

Can we handle the A profile situation where effectively we have v8 + a list of features, with each v8.x and v9.y enabling a set of features https://gcc.gnu.org/onlinedocs/gcc/AArch64-Options.html

Ideally we would want to be able to do things like map v8.5-a+sve+sve2 and v9 to the same libraries.

247

I'm wondering if instead of using the FPU name we use a string representation of the properties for the flags. For example FK_VFPV3_D16 this would be akin to expanding march=armv8.6 to a list of target features. We'd need to make a stable string representation but I think that should be possible.

clang/test/Driver/print-multi-selection-flags.c
41

Would be useful to have some Armv7-A and Armv8-A tests as well. In particular I'm thinking of Arm-v8 and Arm-v8.1 as LSE atomics appear in v8.1.

michaelplatings marked 6 inline comments as done.

Apply improvements suggested by @peter.smith

michaelplatings added inline comments.Feb 7 2023, 3:42 PM
clang/include/clang/Driver/Options.td
4333

The reason is consistency with -print-multi-directory.

clang/lib/Driver/ToolChain.cpp
176–271

Correct, we don't yet need to cater for existing hard-coded Multilibs.

I did think about putting some argument processing in ToolChain subclasses but for now I'm inclined to say YAGNI. If it's needed later on that's an easy refactoring. With the current form, there's less likelihood of different ToolChain subclasses diverging in the style of attributes they emit.

MultilibSet can use a regex, and both matching and not matching can be significant, so I'm not sure it makes sense to filter against that.

-print-multi-selection-flags-experimental can be used completely independently of any YAML or whether the current ToolChain class actually supports multilib. -print-multi-selection-flags-experimental is intended primarily for developing/debugging multilib.

228

Yes. I added AArch64 support and it works exactly as you hoped for v8.5-a+sve+sve2 and v9.

247

ARM::getFPUFeatures does return a list of features like fp-armv8d16 and fullfp16 but AFAIK these names are unstable. I agree a stable string representation should be possible, but unless there's an existing list of stable FPU feature names I think that won't be quick. There's an opportunity cost to not landing this patch so I'd rather not delay that long. This new multilib feature won't be stable until LLVM 18 at the earliest so we can take out the mfpu=... attributes if we can agree on a better representation before then.

clang/test/Driver/print-multi-selection-flags.c
41

I didn't see anything particularly interesting to check about v7a but I added a test for -march=armv8-a+lse

Matt added a subscriber: Matt.Feb 7 2023, 8:32 PM

Thanks for the updates, changes look good to me. I think that we can get away without trying to break the floating point options into flags, at least for how floating point units are available in hardware today.

clang/lib/Driver/ToolChain.cpp
176–271

Thinking about Arm floating point modelling (not really relevant to the mechanism here). Certainly since the Cortex cores the instructions available to an FPU are tied to the architecture. I.e. you cannot mix and match a FPU from an earlier or later architecture revision in practice, even if you can put it on the command-line in theory. I think this means that in the majority of cases we just need to match the floating point libraries with the architecture. The exceptions are the floating point variants with cut-down functionality:

  • single-precision only (a nightmare of hard-float for floats and soft-float for doubles)
  • d16 (only 16 double precision registers available rather than 32. I would expect this to be interface compatible with code compiled with 32-double precision registers as the calling convention doesn't change). All single-precision only are also d16.

I think these fpu options are restricted to Cortex-M and Cortex-R.

For a small number of targets we'd likely need at least two (single only, and d16) and at most 3 (single only, d16, and full) floating point variants. We'd need to map the fpu name suitable for the architecture but I don't think we would want to work too hard trying to map an arbitrary floating point unit that there is no available hardware for.

Add -print-multi-selection-flags argument

"option" is a better term. Since the option doesn't take an argument, "flag" applies as well.

[Driver] Add -print-multi-selection-flags

The summary is empty. Is that intended? We need some description what the new option does.

clang/lib/Driver/ToolChain.cpp
274

remove braces

298

This function overly uses blank lines.

// Sort and remove duplicates.
std::sort(Result.begin(), Result.end());
Result.erase(std::unique(Result.begin(), Result.end()), Result.end());
return Result;
michaelplatings marked 2 inline comments as done.

Remove braces and blank lines as requested by @MaskRay

michaelplatings retitled this revision from Add -print-multi-selection-flags argument to Add -print-multi-selection-flags-experimental option.Feb 22 2023, 4:17 AM
michaelplatings edited the summary of this revision. (Show Details)
michaelplatings removed reviewers: lenary, simon_tatham.

flags -> tags

michaelplatings retitled this revision from Add -print-multi-selection-flags-experimental option to Add -print-multi-selection-tags-experimental option.Mar 9 2023, 1:05 PM
michaelplatings edited the summary of this revision. (Show Details)
peter.smith accepted this revision.Mar 13 2023, 9:23 AM

I've set approved from the Arm side. Please leave some time for people in the US time zone to leave any final comments or ask for extensions.

This revision is now accepted and ready to land.Mar 13 2023, 9:23 AM
phosek added inline comments.Mar 13 2023, 9:40 PM
clang/lib/Driver/ToolChain.cpp
238–246

Would it be possible to add this information to https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Driver/Options.td? I think that would be more maintainable.

clang/lib/Driver/ToolChains/Arch/ARM.cpp
467

Can we use ARM::FPUKind as a type here?

585

Is this variable used anywhere in this function?

clang/lib/Driver/ToolChains/Arch/ARM.h
67

Using output argument isn't very common in LLVM and even in this particular case seems a bit ad-hoc. I'm wondering if we could instead extract this logic into a new function, for example getARMFPUFeatures?

michaelplatings retitled this revision from Add -print-multi-selection-tags-experimental option to Add -print-multi-selection-flags-experimental option.Mar 14 2023, 5:39 AM
michaelplatings edited the summary of this revision. (Show Details)
michaelplatings marked 2 inline comments as done.

Rebase on D146141

clang/lib/Driver/ToolChain.cpp
238–246

I thought about that, and it would be nice - it's a shame that this information gets copy-pasted across the Clang sources - there are already 5 or so places that do this. However for this change I think that would be too much scope creep so I've stuck with the status quo.

clang/lib/Driver/ToolChains/Arch/ARM.cpp
467

I made a new change, D146141, that makes this possible. I've rebased this patch on that change now so this is done.

585

Yes, although previously in this scope only for FPUID == llvm::ARM::FK_NONE. Now we're using it more since we're returning it.

clang/lib/Driver/ToolChains/Arch/ARM.h
67

It's not unusual in this area of the code. Usually the name is ArgFPUKind which I don't find descriptive so I went with OutFPUKind to make it clearer that it's an output of the function. However since the function had return type void I've now simply changed it to return FPUKind directly.

I spent a chunk of time looking into whether I could get a separate FPU-specific function. Finding the FPU is tightly coupled across a lot of code with finding the architecture and CPU. Although I'm sure the code could be better structured, the problem is also inherent because the floating point support is dependent on -mfpu, -march, -mcpu, --target, -mfloat-abi and possibly others so figuring everything out requires parsing many options. In short, I think returning FPUKind is the lesser evil here, even if it does seem a bit ad-hoc.

phosek added inline comments.Jun 6 2023, 10:38 PM
clang/lib/Driver/ToolChain.cpp
250–285

I'd suggest omitting these for the initial version. Ideally we would find a way to specify these more generally through Clang's option parser rather than individually listing all flags here, but that can be done in a follow up change.

phosek added inline comments.Jun 6 2023, 10:40 PM
clang/include/clang/Driver/Options.td
4333

This is just a suggestion, but I'd consider dropping selection, that is the name of the flag would be just -print-multi-flags. It's not clear to me what the "selection" means in this context and I'm not sure if it provides any additional value.

michaelplatings marked 2 inline comments as done.

Rename option and support fewer options as suggested by @phosek

michaelplatings retitled this revision from Add -print-multi-selection-flags-experimental option to [Driver] Add -print-multi-flags-experimental option.Jun 7 2023, 8:23 AM
phosek accepted this revision.Jun 8 2023, 12:38 AM

LGTM

MaskRay accepted this revision.Jun 8 2023, 2:49 PM
MaskRay added inline comments.
clang/lib/Driver/ToolChain.cpp
266
This revision was landed with ongoing or failed builds.Jun 13 2023, 10:47 PM
This revision was automatically updated to reflect the committed changes.
michaelplatings marked an inline comment as done.Jun 13 2023, 10:50 PM