[PATCH] D142933: Add -print-multi-selection-flags-experimental option

[Michael Platings via Phabricator via cfe-commits]

michaelplatings added inline comments.


================
Comment at: clang/lib/Driver/ToolChain.cpp:237-245
+  // Enumerate boolean flags we care about for the purposes of multilib here.
+  // There must be a smarter way to do it but this gets us started.
+  const struct HasFlag {
+    ID Pos, Neg;
+    bool Default;
+  } HasFlagList[] = {
+      {OPT_fexceptions, OPT_fno_exceptions, true},
----------------
phosek wrote:
> 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.
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.


================
Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:441
+                               std::vector<StringRef> &Features, bool ForAS,
+                               unsigned *OutFPUKind) {
   bool KernelOrKext =
----------------
phosek wrote:
> Can we use `ARM::FPUKind` as a type here?
I made a new change, D146141, that makes this possible. I've rebased this patch on that change now so this is done.


================
Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:556
   // Honor -mfpu=. ClangAs gives preference to -Wa,-mfpu=.
   unsigned FPUID = llvm::ARM::FK_INVALID;
   const Arg *FPUArg = Args.getLastArg(options::OPT_mfpu_EQ);
----------------
phosek wrote:
> Is this variable used anywhere in this function?
Yes, although previously in this scope only for `FPUID == llvm::ARM::FK_NONE`. Now we're using it more since we're returning it.


================
Comment at: clang/lib/Driver/ToolChains/Arch/ARM.h:70
+                          std::vector<llvm::StringRef> &Features, bool ForAS,
+                          unsigned *OutFPUKind = nullptr);
 int getARMSubArchVersionNumber(const llvm::Triple &Triple);
----------------
phosek wrote:
> 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`?
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142933