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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
