[PATCH] D82948: [Driver][ARM] Disable unsupported features when nofp arch extension is used

[Victor Campos via Phabricator via cfe-commits]

vhscampos marked 2 inline comments as done.
vhscampos added a comment.

I will merge the two patches into one.

Please also see my inline responses.



================
Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:288
 
+static void appendNoFPUnsupportedFeatures(const arm::FloatABI ABI,
+                                          const unsigned FPUID,
----------------
chill wrote:
> That's kinda mouthful name.
Not sure how to compress it more without making it unmeaningful though.


================
Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:292-297
+  auto checkFPDisabledInArchName = [](const StringRef &ArchName) {
+    SmallVector<StringRef, 8> Split;
+    ArchName.split(Split, '+', -1, false);
+    return llvm::any_of(
+        Split, [](const StringRef &Extension) { return Extension == "nofp"; });
+  };
----------------
chill wrote:
> chill wrote:
> > Wouldn't just looking for the substring do the job?
> > 
> > Also need to handle `-mcpu=...+nofp`.
> > 
> > We already "parse" the arguments to `-march=` and `-mcpu=` (and `-mfpu=`) earlier, it seems to me we
> > could note the `+nofp` and `+nofp.dp` earlier. (TBH, it isn't immediately obvious to me how to untangle this mess).
> > 
> Hmm, actually, `+nofp.dp` should not disable the FPU, I think.
Just looking for the substring might be sufficient indeed.

Yes, we already do `-march`/`-mcpu` parsing a bit earlier. However, this parsing and the following handling of it is done deeper in the call stack. I wondered about ways to propagate this information back to this point here (e.g. adding one more by-ref argument that is set by the first round of parsing), but I don't feel confident to back it up.

Are you okay with me just changing it to a substring search?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82948