t.p.northover added inline comments.
================
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")) {
----------------
simon_tatham wrote:
> 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)?
Ah yes, I see. I think your alternative is probably better.
================
Comment at: llvm/lib/Support/ARMTargetParser.cpp:504-507
+ if (ArchExt == "fp" || ArchExt == "fp.dp") {
+ unsigned FPUKind;
+ if (Negated) {
+ FPUKind = ARM::FK_NONE;
----------------
simon_tatham wrote:
> 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?
I think I'd go for a diagnostic in that case. There's already a way to strip
out the FPU then (`+nofp`).
================
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
+
----------------
simon_tatham wrote:
> 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'
> ```
Ah, I missed the only way return true could happen and assumed the return value
was vestigial. Sorry.
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
