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

[Peter Smith via Phabricator via cfe-commits]

peter.smith 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)">;
----------------
any particular reason for using multi rather than multilib? Is there any intent to use this anywhere else?


================
Comment at: clang/include/clang/Driver/ToolChain.h:289
+  /// Get flags suitable for multilib selection, based on the provided clang
+  /// command line arguments. The arguments aren't suitable to be used directly
+  /// for multilib selection because they are not normalized and normalization
----------------
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.


================
Comment at: clang/include/clang/Driver/ToolChain.h:303
+  ///   returned separately. There may not be valid syntax to express this as a
+  ///   clang argument so an pseudo argument syntax must be used instead.
+  /// To allow users to find out what flags are returned, clang accepts a
----------------
typo "a pseudo"


================
Comment at: clang/lib/Driver/ToolChain.cpp:173
 }
 
+std::vector<std::string>
----------------
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.


================
Comment at: clang/lib/Driver/ToolChain.cpp:220
+
+  switch (Triple.getArch()) {
+  case llvm::Triple::arm:
----------------
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. 
```


================
Comment at: clang/lib/Driver/ToolChain.cpp:225
+  case llvm::Triple::thumbeb: {
+    std::vector<StringRef> Features;
+    unsigned FPUKind = llvm::ARM::FK_INVALID;
----------------
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. 


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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142933