simon_tatham marked 3 inline comments as done. simon_tatham added a comment.
The aim of this change is that it will apply to the v8.1-M (mainline) architecture introduced in D60698 <https://reviews.llvm.org/D60698>, in which `+fp` //won't// be the default: `-march=armv8.1m.main` by itself gives you the base 8.1-M architecture without any FP, `-march=armv8.1m.main+fp` gives you the optional single-precision FP extension on top of that, and `+fp.dp` gives you double precision as well. ================ Comment at: llvm/lib/Support/ARMTargetParser.cpp:476 -StringRef ARM::getArchExtFeature(StringRef ArchExt) { - if (ArchExt.startswith("no")) { - StringRef ArchExtBase(ArchExt.substr(2)); - for (const auto AE : ARCHExtNames) { - if (AE.NegFeature && ArchExtBase == AE.getName()) - return StringRef(AE.NegFeature); - } +static bool wasNegated(StringRef &Name) { + if (Name.startswith("no")) { ---------------- t.p.northover wrote: > I think `isNegated` is probably more in line with existing naming. Hmmm. I thought that would be a confusing name because it hides the fact that the function strips off the `no` prefix. (The use of 'was' was intended to hint that by the time the function returns, it's not true any more!) Perhaps `stripNegationPrefix` (returning bool to indicate success)? ================ Comment at: llvm/lib/Support/ARMTargetParser.cpp:504-507 + if (ArchExt == "fp" || ArchExt == "fp.dp") { + unsigned FPUKind; + if (Negated) { + FPUKind = ARM::FK_NONE; ---------------- t.p.northover wrote: > Doesn't this mean `+nofp.dp` disables all floats? That seems surprising > behaviour to me, but I can't find any existing tests covering it. Hmmm, that's a good point. What //would// a user expect in that situation? If double-precision FP was the default for that architecture and a single-precision version existed, then perhaps `nofp.dp` should fall back to that, but what if it's double or nothing? ================ Comment at: llvm/lib/Support/ARMTargetParser.cpp:516-517 + const FPUName &DefaultFPU = FPUNames[FPUKind]; + if (DefaultFPU.Restriction != FPURestriction::SP_D16) + return false; // meaningless for this arch + ---------------- t.p.northover wrote: > Doesn't this silently disable the FPU entirely if we decide "fp.dp" is > useless? That seems unlikely to be what a user wants, especially without a > diagnostic. > > Could you also expand on the comment a bit more. I had to look up exactly > what FPURestrictions existed to get this, and I'm not even 100% sure I'm > right now. I don't think it //silently// disables it, does it? Returning false from this function is a failure indication that ends up back in `checkARMArchName` in `clang/lib/Driver/ToolChains/Arch/ARM.cpp`, which will generate a diagnostic. For example, if I try `-march=armv6m+fp.dp` then I see ``` error: the clang compiler does not support '-march=armv6m+fp.dp' ``` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60697/new/ https://reviews.llvm.org/D60697 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
