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

[Momchil Velikov via Phabricator via cfe-commits]

chill added inline comments.


================
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"; });
+  };
----------------
vhscampos wrote:
> vhscampos wrote:
> > 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?
> Actually it may be better to keep the string splitting method. The search required here must be whole-word, as to flag up "+nofp", but not "+nofp.dp". It can be done with less code using the current list of tokens as opposed to using substring search, followed by a "is it whole-word?" check.
In fact, it's less number of tokens, not that number of tokens is that important.

    auto anyNOFP = [](const StringRef &str) {
      size_t pos = str.find_lower("+nofp");
      return pos != StringRef::npos &&
              (pos + 5 == str.size() || str[pos + 5] == '+');
    };

Or it could become

    auto findFeature = [](const StringRef &str, const StringRef &feature) {
      size_t pos = str.find_lower(feature);
      return (pos == StringRef::npos || pos + feature.size() == str.size() ||
              str[pos + feature.size()] == '+')
                 ? pos
                 : StringRef::npos;
    };

which is slightly longer, but more universal and allows you to check relative positions of features.

A-a-a-nyway, I think this string twiddling should be the absolute last resort. 

Could you please, see, if `checkARMArchName` and `checkARMCpuName` can be tweaked into
returning `llvm::ARM::FK_NONE` or `llvm::ARM::FK_INVALID` or whatever, depending on what is present
in option value of `-march=...` and `-mcpu=...`?

An interesting question is what to do if there are contradictions, but I think the general strategy is to not strive to produce sensible output from nonsensical input.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82948