[PATCH] D142933: Add -print-multi-selection-flags argument

Tue Feb 7 15:42:10 PST 2023

michaelplatings added inline comments.

================
Comment at: clang/include/clang/Driver/Options.td:4162
 def print_multi_lib : Flag<["-", "--"], "print-multi-lib">;
+def print_multi_selection_flags : Flag<["-", "--"], "print-multi-selection-flags-experimental">,
+  HelpText<"Print the flags used for selecting multilibs (experimental)">;
----------------
peter.smith wrote:
> any particular reason for using multi rather than multilib? Is there any intent to use this anywhere else?
The reason is consistency with -print-multi-directory.

================
Comment at: clang/lib/Driver/ToolChain.cpp:173
 }

+std::vector<std::string>
----------------
peter.smith wrote:
> 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.
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.

================
Comment at: clang/lib/Driver/ToolChain.cpp:225
+  case llvm::Triple::thumbeb: {
+    std::vector<StringRef> Features;
+    unsigned FPUKind = llvm::ARM::FK_INVALID;
----------------
peter.smith wrote:
> 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. 
Yes. I added AArch64 support and it works exactly as you hoped for v8.5-a+sve+sve2 and v9.

================
Comment at: clang/lib/Driver/ToolChain.cpp:244
+#include "llvm/TargetParser/ARMTargetParser.def"
+    default:
+      llvm_unreachable("Invalid FPUKind");
----------------
peter.smith wrote:
> 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.
`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.

================
Comment at: clang/test/Driver/print-multi-selection-flags.c:40
+// CHECK-MVE: target=thumbv8.1m.main-none-unknown-eabihf
+
+// RUN: %clang -print-multi-selection-flags-experimental --target=arm-none-eabi -march=armv8.1m.main+mve+nofp | FileCheck --check-prefix=CHECK-MVENOFP %s
----------------
peter.smith wrote:
> 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. 
I didn't see anything particularly interesting to check about v7a but I added a test for `-march=armv8-a+lse`

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D142933/new/

https://reviews.llvm.org/D142933