[PATCH] D60697: [ARM] Allow "-march=foo+fp" to vary with foo.

[Sjoerd Meijer via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL362600: [ARM] Allow -march=foo+fp to vary with 
foo (authored by SjoerdMeijer, committed by ).
Herald added a subscriber: kristina.

Changed prior to commit:
  https://reviews.llvm.org/D60697?vs=202934=203129#toc

Repository:
  rL LLVM

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

https://reviews.llvm.org/D60697

Files:
  llvm/trunk/include/llvm/Support/ARMTargetParser.h
  llvm/trunk/lib/Support/ARMTargetParser.cpp

Index: llvm/trunk/include/llvm/Support/ARMTargetParser.h
===
--- llvm/trunk/include/llvm/Support/ARMTargetParser.h
+++ llvm/trunk/include/llvm/Support/ARMTargetParser.h
@@ -240,6 +240,8 @@
 StringRef getSubArch(ArchKind AK);
 StringRef getArchExtName(unsigned ArchExtKind);
 StringRef getArchExtFeature(StringRef ArchExt);
+bool appendArchExtFeatures(StringRef CPU, ARM::ArchKind AK, StringRef ArchExt,
+   std::vector );
 StringRef getHWDivName(unsigned HWDivKind);
 
 // Information by Name
Index: llvm/trunk/lib/Support/ARMTargetParser.cpp
===
--- llvm/trunk/lib/Support/ARMTargetParser.cpp
+++ llvm/trunk/lib/Support/ARMTargetParser.cpp
@@ -485,22 +485,85 @@
   return StringRef();
 }
 
-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 stripNegationPrefix(StringRef ) {
+  if (Name.startswith("no")) {
+Name = Name.substr(2);
+return true;
   }
+  return false;
+}
+
+StringRef ARM::getArchExtFeature(StringRef ArchExt) {
+  bool Negated = stripNegationPrefix(ArchExt);
   for (const auto AE : ARCHExtNames) {
 if (AE.Feature && ArchExt == AE.getName())
-  return StringRef(AE.Feature);
+  return StringRef(Negated ? AE.NegFeature : AE.Feature);
   }
 
   return StringRef();
 }
 
+static unsigned findDoublePrecisionFPU(unsigned InputFPUKind) {
+  const ARM::FPUName  = ARM::FPUNames[InputFPUKind];
+
+  // If the input FPU already supports double-precision, then there
+  // isn't any different FPU we can return here.
+  //
+  // The current available FPURestriction values are None (no
+  // restriction), D16 (only 16 d-regs) and SP_D16 (16 d-regs
+  // and single precision only); there's no value representing
+  // SP restriction without D16. So this test just means 'is it
+  // SP only?'.
+  if (InputFPU.Restriction != ARM::FPURestriction::SP_D16)
+return ARM::FK_INVALID;
+
+  // Otherwise, look for an FPU entry with all the same fields, except
+  // that SP_D16 has been replaced with just D16, representing adding
+  // double precision and not changing anything else.
+  for (const ARM::FPUName  : ARM::FPUNames) {
+if (CandidateFPU.FPUVer == InputFPU.FPUVer &&
+CandidateFPU.NeonSupport == InputFPU.NeonSupport &&
+CandidateFPU.Restriction == ARM::FPURestriction::D16) {
+  return CandidateFPU.ID;
+}
+  }
+
+  // nothing found
+  return ARM::FK_INVALID;
+}
+
+bool ARM::appendArchExtFeatures(
+  StringRef CPU, ARM::ArchKind AK, StringRef ArchExt,
+  std::vector ) {
+  StringRef StandardFeature = getArchExtFeature(ArchExt);
+  if (!StandardFeature.empty()) {
+Features.push_back(StandardFeature);
+return true;
+  }
+
+  const bool Negated = stripNegationPrefix(ArchExt);
+
+  if (CPU == "")
+CPU = "generic";
+
+  if (ArchExt == "fp" || ArchExt == "fp.dp") {
+unsigned FPUKind;
+if (ArchExt == "fp.dp") {
+  if (Negated) {
+Features.push_back("-fp64");
+return true;
+  }
+  FPUKind = findDoublePrecisionFPU(getDefaultFPU(CPU, AK));
+} else if (Negated) {
+  FPUKind = ARM::FK_NONE;
+} else {
+  FPUKind = getDefaultFPU(CPU, AK);
+}
+return ARM::getFPUFeatures(FPUKind, Features);
+  }
+  return false;
+}
+
 StringRef ARM::getHWDivName(unsigned HWDivKind) {
   for (const auto D : HWDivNames) {
 if (HWDivKind == D.ID)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D60697: [ARM] Allow "-march=foo+fp" to vary with foo.

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard accepted this revision.
ostannard added a comment.
This revision is now accepted and ready to land.

Fair enough, I don't think we currently try to diagnose any other invalid 
combinations of features. LGTM.


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

[PATCH] D60697: [ARM] Allow "-march=foo+fp" to vary with foo.

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer updated this revision to Diff 202934.
SjoerdMeijer added a comment.

Hi Oliver, thanks for your comments!

This was the easy one, they have been added:

> I also don't see any tests for the negated forms of either feature.

The trouble begun with this:

> +fp.dp, but the FPU is already double-precision
>  +fp.dp, but no double-precision FPU exists (are there any FPUs which cause 
> this?)
>  +[no]fp or +[no]fp.dp for a CPU/arch which doesn't have any FPUs.

Because I found that basically none of this worked. The main reason was that we 
were always passing `generic`. To address that we at least have a chance of 
seeing a sensible CPU name, I have swapped the order of parsing -march and 
-mcpu. I.e., we parse -mcpu first, and pass that to `checkARMArchName`, which 
will eventually call `appendArchExtFeatures`. I think that makes more sense 
when we use the CPUname to query `getDefaultFPU`.

Then about the more fancy diagnostics (e.g. "fp.dp, but the FPU is already 
double-precision"): I've removed any attempt to throw clever diagnostics. I 
don't think, in general, that we provide this kind of service level. I.e., we 
need to do a lot more work here to avoid a meaningless, confusing, and thus 
useless  "--march=... not supported" error message when we provide +fp.dp on 
the -march when e.g. the CPU already enabled this.


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

https://reviews.llvm.org/D60697

Files:
  clang/lib/Driver/ToolChains/Arch/ARM.cpp
  clang/lib/Driver/ToolChains/Arch/ARM.h
  clang/test/Driver/armv8.1m.main.c
  clang/test/Driver/armv8.1m.main.s
  llvm/include/llvm/Support/ARMTargetParser.h
  llvm/lib/Support/ARMTargetParser.cpp

Index: llvm/lib/Support/ARMTargetParser.cpp
===
--- llvm/lib/Support/ARMTargetParser.cpp
+++ llvm/lib/Support/ARMTargetParser.cpp
@@ -484,22 +484,85 @@
   return StringRef();
 }
 
-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 stripNegationPrefix(StringRef ) {
+  if (Name.startswith("no")) {
+Name = Name.substr(2);
+return true;
   }
+  return false;
+}
+
+StringRef ARM::getArchExtFeature(StringRef ArchExt) {
+  bool Negated = stripNegationPrefix(ArchExt);
   for (const auto AE : ARCHExtNames) {
 if (AE.Feature && ArchExt == AE.getName())
-  return StringRef(AE.Feature);
+  return StringRef(Negated ? AE.NegFeature : AE.Feature);
   }
 
   return StringRef();
 }
 
+static unsigned findDoublePrecisionFPU(unsigned InputFPUKind) {
+  const ARM::FPUName  = ARM::FPUNames[InputFPUKind];
+
+  // If the input FPU already supports double-precision, then there
+  // isn't any different FPU we can return here.
+  //
+  // The current available FPURestriction values are None (no
+  // restriction), D16 (only 16 d-regs) and SP_D16 (16 d-regs
+  // and single precision only); there's no value representing
+  // SP restriction without D16. So this test just means 'is it
+  // SP only?'.
+  if (InputFPU.Restriction != ARM::FPURestriction::SP_D16)
+return ARM::FK_INVALID;
+
+  // Otherwise, look for an FPU entry with all the same fields, except
+  // that SP_D16 has been replaced with just D16, representing adding
+  // double precision and not changing anything else.
+  for (const ARM::FPUName  : ARM::FPUNames) {
+if (CandidateFPU.FPUVer == InputFPU.FPUVer &&
+CandidateFPU.NeonSupport == InputFPU.NeonSupport &&
+CandidateFPU.Restriction == ARM::FPURestriction::D16) {
+  return CandidateFPU.ID;
+}
+  }
+
+  // nothing found
+  return ARM::FK_INVALID;
+}
+
+bool ARM::appendArchExtFeatures(
+  StringRef CPU, ARM::ArchKind AK, StringRef ArchExt,
+  std::vector ) {
+  StringRef StandardFeature = getArchExtFeature(ArchExt);
+  if (!StandardFeature.empty()) {
+Features.push_back(StandardFeature);
+return true;
+  }
+
+  const bool Negated = stripNegationPrefix(ArchExt);
+
+  if (CPU == "")
+CPU = "generic";
+
+  if (ArchExt == "fp" || ArchExt == "fp.dp") {
+unsigned FPUKind;
+if (ArchExt == "fp.dp") {
+  if (Negated) {
+Features.push_back("-fp64");
+return true;
+  }
+  FPUKind = findDoublePrecisionFPU(getDefaultFPU(CPU, AK));
+} else if (Negated) {
+  FPUKind = ARM::FK_NONE;
+} else {
+  FPUKind = getDefaultFPU(CPU, AK);
+}
+return ARM::getFPUFeatures(FPUKind, Features);
+  }
+  return false;
+}
+
 StringRef ARM::getHWDivName(unsigned HWDivKind) {
   for (const auto D : HWDivNames) {
 if (HWDivKind == D.ID)
Index: llvm/include/llvm/Support/ARMTargetParser.h
===
--- llvm/include/llvm/Support/ARMTargetParser.h
+++ llvm/include/llvm/Support/ARMTargetParser.h
@@ 

[PATCH] D60697: [ARM] Allow "-march=foo+fp" to vary with foo.

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard added inline comments.



Comment at: llvm/lib/Support/ARMTargetParser.cpp:551
+
+  // If the default FPU already supports double-precision, or if
+  // there is no double-prec FPU that extends it, then "fp.dp"

Could you also add tests for the error cases here? I think these are:
* +fp.dp, but the FPU is already double-precision
* +fp.dp, but no double-precision FPU exists (are there any FPUs which cause 
this?)
* +[no]fp or +[no]fp.dp for a CPU/arch which doesn't have any FPUs.

I also don't see any tests for the negated forms of either feature.


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

[PATCH] D60697: [ARM] Allow "-march=foo+fp" to vary with foo.

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer updated this revision to Diff 202433.
SjoerdMeijer added a comment.

This time with tests.


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

https://reviews.llvm.org/D60697

Files:
  clang/lib/Driver/ToolChains/Arch/ARM.cpp
  clang/lib/Driver/ToolChains/Arch/ARM.h
  clang/test/Driver/armv8.1m.main.c
  clang/test/Driver/armv8.1m.main.s
  llvm/include/llvm/Support/ARMTargetParser.h
  llvm/lib/Support/ARMTargetParser.cpp

Index: llvm/lib/Support/ARMTargetParser.cpp
===
--- llvm/lib/Support/ARMTargetParser.cpp
+++ llvm/lib/Support/ARMTargetParser.cpp
@@ -484,22 +484,100 @@
   return StringRef();
 }
 
-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 stripNegationPrefix(StringRef ) {
+  if (Name.startswith("no")) {
+Name = Name.substr(2);
+return true;
   }
+  return false;
+}
+
+StringRef ARM::getArchExtFeature(StringRef ArchExt) {
+  bool Negated = stripNegationPrefix(ArchExt);
   for (const auto AE : ARCHExtNames) {
 if (AE.Feature && ArchExt == AE.getName())
-  return StringRef(AE.Feature);
+  return StringRef(Negated ? AE.NegFeature : AE.Feature);
   }
 
   return StringRef();
 }
 
+static unsigned findDoublePrecisionFPU(unsigned InputFPUKind) {
+  const ARM::FPUName  = ARM::FPUNames[InputFPUKind];
+
+  // If the input FPU already supports double-precision, then there
+  // isn't any different FPU we can return here.
+  //
+  // The current available FPURestriction values are None (no
+  // restriction), D16 (only 16 d-regs) and SP_D16 (16 d-regs
+  // and single precision only); there's no value representing
+  // SP restriction without D16. So this test just means 'is it
+  // SP only?'.
+  if (InputFPU.Restriction != ARM::FPURestriction::SP_D16)
+return ARM::FK_INVALID;
+
+  // Otherwise, look for an FPU entry with all the same fields, except
+  // that SP_D16 has been replaced with just D16, representing adding
+  // double precision and not changing anything else.
+  for (const ARM::FPUName  : ARM::FPUNames) {
+if (CandidateFPU.FPUVer == InputFPU.FPUVer &&
+CandidateFPU.NeonSupport == InputFPU.NeonSupport &&
+CandidateFPU.Restriction == ARM::FPURestriction::D16) {
+  return CandidateFPU.ID;
+}
+  }
+
+  // nothing found
+  return ARM::FK_INVALID;
+}
+
+bool ARM::appendArchExtFeatures(
+  StringRef CPU, ARM::ArchKind AK, StringRef ArchExt,
+  std::vector ) {
+  StringRef StandardFeature = getArchExtFeature(ArchExt);
+  if (!StandardFeature.empty()) {
+Features.push_back(StandardFeature);
+return true;
+  }
+
+  bool Negated = stripNegationPrefix(ArchExt);
+
+  if (ArchExt == "fp" || ArchExt == "fp.dp") {
+unsigned FPUKind;
+
+if (ArchExt == "fp.dp") {
+  unsigned DoubleFPUKind = findDoublePrecisionFPU(getDefaultFPU(CPU, AK));
+
+  // If the default FPU already supports double-precision, or if
+  // there is no double-prec FPU that extends it, then "fp.dp"
+  // doesn't have a separate meaning, and we treat it as an
+  // invalid extension name.
+  if (DoubleFPUKind == FK_INVALID)
+return false;
+
+  // If there _is_ a separate double-precision FPU, then "nofp.dp"
+  // should disable just the double-precision extension, leaving
+  // the base FPU still enabled if it previously was.
+  if (Negated) {
+Features.push_back("-fp64");
+return true;
+  }
+
+  // Otherwise, select the double-precision FPU.
+  FPUKind = DoubleFPUKind;
+} else if (Negated) {
+  FPUKind = ARM::FK_NONE;
+} else {
+  FPUKind = getDefaultFPU(CPU, AK);
+  if (FPUKind == ARM::FK_NONE)
+return false;
+}
+return ARM::getFPUFeatures(FPUKind, Features);
+  }
+
+  return false;
+}
+
 StringRef ARM::getHWDivName(unsigned HWDivKind) {
   for (const auto D : HWDivNames) {
 if (HWDivKind == D.ID)
Index: llvm/include/llvm/Support/ARMTargetParser.h
===
--- llvm/include/llvm/Support/ARMTargetParser.h
+++ llvm/include/llvm/Support/ARMTargetParser.h
@@ -240,6 +240,8 @@
 StringRef getSubArch(ArchKind AK);
 StringRef getArchExtName(unsigned ArchExtKind);
 StringRef getArchExtFeature(StringRef ArchExt);
+bool appendArchExtFeatures(StringRef CPU, ARM::ArchKind AK, StringRef ArchExt,
+   std::vector );
 StringRef getHWDivName(unsigned HWDivKind);
 
 // Information by Name
Index: clang/test/Driver/armv8.1m.main.s
===
--- clang/test/Driver/armv8.1m.main.s
+++ clang/test/Driver/armv8.1m.main.s
@@ -5,8 +5,14 @@
 # RUN:  FileCheck --check-prefix=ERROR-V81M < %t %s
 # RUN: not %clang -c 

[PATCH] D60697: [ARM] Allow "-march=foo+fp" to vary with foo.

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer marked an inline comment as done.
SjoerdMeijer added a comment.

Ah yes, the school boy error! ;-) Actually, there was a test, but in a 
different patch; I will move it to here.


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

[PATCH] D60697: [ARM] Allow "-march=foo+fp" to vary with foo.

[Oliver Stannard (Linaro) via Phabricator via cfe-commits]

ostannard requested changes to this revision.
ostannard added a comment.
This revision now requires changes to proceed.

This still needs tests adding.


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

[PATCH] D60697: [ARM] Allow "-march=foo+fp" to vary with foo.

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer updated this revision to Diff 202388.
SjoerdMeijer added a comment.

This addresses @t.p.northover comment.


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

https://reviews.llvm.org/D60697

Files:
  clang/lib/Driver/ToolChains/Arch/ARM.cpp
  clang/lib/Driver/ToolChains/Arch/ARM.h
  llvm/include/llvm/Support/ARMTargetParser.h
  llvm/lib/Support/ARMTargetParser.cpp

Index: llvm/lib/Support/ARMTargetParser.cpp
===
--- llvm/lib/Support/ARMTargetParser.cpp
+++ llvm/lib/Support/ARMTargetParser.cpp
@@ -484,22 +484,100 @@
   return StringRef();
 }
 
-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 stripNegationPrefix(StringRef ) {
+  if (Name.startswith("no")) {
+Name = Name.substr(2);
+return true;
   }
+  return false;
+}
+
+StringRef ARM::getArchExtFeature(StringRef ArchExt) {
+  bool Negated = stripNegationPrefix(ArchExt);
   for (const auto AE : ARCHExtNames) {
 if (AE.Feature && ArchExt == AE.getName())
-  return StringRef(AE.Feature);
+  return StringRef(Negated ? AE.NegFeature : AE.Feature);
   }
 
   return StringRef();
 }
 
+static unsigned findDoublePrecisionFPU(unsigned InputFPUKind) {
+  const ARM::FPUName  = ARM::FPUNames[InputFPUKind];
+
+  // If the input FPU already supports double-precision, then there
+  // isn't any different FPU we can return here.
+  //
+  // The current available FPURestriction values are None (no
+  // restriction), D16 (only 16 d-regs) and SP_D16 (16 d-regs
+  // and single precision only); there's no value representing
+  // SP restriction without D16. So this test just means 'is it
+  // SP only?'.
+  if (InputFPU.Restriction != ARM::FPURestriction::SP_D16)
+return ARM::FK_INVALID;
+
+  // Otherwise, look for an FPU entry with all the same fields, except
+  // that SP_D16 has been replaced with just D16, representing adding
+  // double precision and not changing anything else.
+  for (const ARM::FPUName  : ARM::FPUNames) {
+if (CandidateFPU.FPUVer == InputFPU.FPUVer &&
+CandidateFPU.NeonSupport == InputFPU.NeonSupport &&
+CandidateFPU.Restriction == ARM::FPURestriction::D16) {
+  return CandidateFPU.ID;
+}
+  }
+
+  // nothing found
+  return ARM::FK_INVALID;
+}
+
+bool ARM::appendArchExtFeatures(
+  StringRef CPU, ARM::ArchKind AK, StringRef ArchExt,
+  std::vector ) {
+  StringRef StandardFeature = getArchExtFeature(ArchExt);
+  if (!StandardFeature.empty()) {
+Features.push_back(StandardFeature);
+return true;
+  }
+
+  bool Negated = stripNegationPrefix(ArchExt);
+
+  if (ArchExt == "fp" || ArchExt == "fp.dp") {
+unsigned FPUKind;
+
+if (ArchExt == "fp.dp") {
+  unsigned DoubleFPUKind = findDoublePrecisionFPU(getDefaultFPU(CPU, AK));
+
+  // If the default FPU already supports double-precision, or if
+  // there is no double-prec FPU that extends it, then "fp.dp"
+  // doesn't have a separate meaning, and we treat it as an
+  // invalid extension name.
+  if (DoubleFPUKind == FK_INVALID)
+return false;
+
+  // If there _is_ a separate double-precision FPU, then "nofp.dp"
+  // should disable just the double-precision extension, leaving
+  // the base FPU still enabled if it previously was.
+  if (Negated) {
+Features.push_back("-fp64");
+return true;
+  }
+
+  // Otherwise, select the double-precision FPU.
+  FPUKind = DoubleFPUKind;
+} else if (Negated) {
+  FPUKind = ARM::FK_NONE;
+} else {
+  FPUKind = getDefaultFPU(CPU, AK);
+  if (FPUKind == ARM::FK_NONE)
+return false;
+}
+return ARM::getFPUFeatures(FPUKind, Features);
+  }
+
+  return false;
+}
+
 StringRef ARM::getHWDivName(unsigned HWDivKind) {
   for (const auto D : HWDivNames) {
 if (HWDivKind == D.ID)
Index: llvm/include/llvm/Support/ARMTargetParser.h
===
--- llvm/include/llvm/Support/ARMTargetParser.h
+++ llvm/include/llvm/Support/ARMTargetParser.h
@@ -240,6 +240,8 @@
 StringRef getSubArch(ArchKind AK);
 StringRef getArchExtName(unsigned ArchExtKind);
 StringRef getArchExtFeature(StringRef ArchExt);
+bool appendArchExtFeatures(StringRef CPU, ARM::ArchKind AK, StringRef ArchExt,
+   std::vector );
 StringRef getHWDivName(unsigned HWDivKind);
 
 // Information by Name
Index: clang/lib/Driver/ToolChains/Arch/ARM.h
===
--- clang/lib/Driver/ToolChains/Arch/ARM.h
+++ clang/lib/Driver/ToolChains/Arch/ARM.h
@@ -13,6 +13,7 @@
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/Triple.h"
 #include "llvm/Option/Option.h"
+#include 

[PATCH] D60697: [ARM] Allow "-march=foo+fp" to vary with foo.

[Tim Northover via Phabricator via cfe-commits]

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 ) {
+  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  = 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

[PATCH] D60697: [ARM] Allow "-march=foo+fp" to vary with foo.

[Simon Tatham via Phabricator via cfe-commits]

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 , 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 ) {
+  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  = 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

[PATCH] D60697: [ARM] Allow "-march=foo+fp" to vary with foo.

[Tim Northover via Phabricator via cfe-commits]

t.p.northover added a comment.

This needs some tests. I'm also not quite sure when you'd use bare "+fp", if 
it's the default anyway.




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 ) {
+  if (Name.startswith("no")) {

I think `isNegated` is probably more in line with existing naming.



Comment at: llvm/lib/Support/ARMTargetParser.cpp:504-507
+  if (ArchExt == "fp" || ArchExt == "fp.dp") {
+unsigned FPUKind;
+if (Negated) {
+  FPUKind = ARM::FK_NONE;

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.



Comment at: llvm/lib/Support/ARMTargetParser.cpp:516-517
+const FPUName  = FPUNames[FPUKind];
+if (DefaultFPU.Restriction != FPURestriction::SP_D16)
+  return false;// meaningless for this arch
+

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.


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

[PATCH] D60697: [ARM] Allow "-march=foo+fp" to vary with foo.

[Simon Tatham via Phabricator via cfe-commits]

simon_tatham created this revision.
simon_tatham added reviewers: dmgreen, samparker, SjoerdMeijer.
Herald added subscribers: llvm-commits, cfe-commits, hiraditya, kristof.beyls, 
javed.absar.
Herald added projects: clang, LLVM.

Now, when clang processes an argument of the form "-march=foo+x+y+z",
then instead of calling getArchExtFeature() for each of the extension
names "x", "y", "z" and appending the returned string to its list of
low-level subtarget features, it will call appendArchExtFeatures()
which does the appending itself.

The difference is that appendArchExtFeatures can add _more_ than one
low-level feature name to the output feature list if it has to, and
also, it gets told some information about what base architecture and
CPU the extension is going to go with, which means that "+fp" can now
mean something different for different CPUs. Namely, "+fp" now selects
whatever the _default_ FPU is for the selected CPU and/or
architecture, as defined in the ARM_ARCH or ARM_CPU_NAME macros in
ARMTargetParser.def.

On the clang side, I adjust DecodeARMFeatures to call the new
appendArchExtFeatures function in place of getArchExtFeature. This
means DecodeARMFeatures needs to be passed a CPU name and an ArchKind,
which meant changing its call sites to make those available, and also
sawing getLLVMArchSuffixForARM in half so that you can get an ArchKind
enum value out of it instead of a string.

Also, I add support here for the extension name "+fp.dp", which will
automatically look through the FPU list for something that looks just
like the default FPU except for also supporting double precision.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D60697

Files:
  clang/lib/Driver/ToolChains/Arch/ARM.cpp
  clang/lib/Driver/ToolChains/Arch/ARM.h
  llvm/include/llvm/Support/ARMTargetParser.h
  llvm/lib/Support/ARMTargetParser.cpp

Index: llvm/lib/Support/ARMTargetParser.cpp
===
--- llvm/lib/Support/ARMTargetParser.cpp
+++ llvm/lib/Support/ARMTargetParser.cpp
@@ -473,22 +473,68 @@
   return StringRef();
 }
 
-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 ) {
+  if (Name.startswith("no")) {
+Name = Name.substr(2);
+return true;
   }
+  return false;
+}
+
+StringRef ARM::getArchExtFeature(StringRef ArchExt) {
+  bool Negated = wasNegated(ArchExt);
   for (const auto AE : ARCHExtNames) {
 if (AE.Feature && ArchExt == AE.getName())
-  return StringRef(AE.Feature);
+  return StringRef(Negated ? AE.NegFeature : AE.Feature);
   }
 
   return StringRef();
 }
 
+bool ARM::appendArchExtFeatures(
+  StringRef CPU, ARM::ArchKind AK, StringRef ArchExt,
+  std::vector ) {
+  StringRef StandardFeature = getArchExtFeature(ArchExt);
+  if (!StandardFeature.empty()) {
+Features.push_back(StandardFeature);
+return true;
+  }
+
+  bool Negated = wasNegated(ArchExt);
+  if (ArchExt == "fp" || ArchExt == "fp.dp") {
+unsigned FPUKind;
+if (Negated) {
+  FPUKind = ARM::FK_NONE;
+} else {
+  FPUKind = getDefaultFPU(CPU, AK);
+  if (FPUKind == ARM::FK_NONE)
+return false;
+  if (ArchExt == "fp.dp") {
+// Enable a double-precision-capable FPU in cases where the
+// default one is single-precision only.
+const FPUName  = FPUNames[FPUKind];
+if (DefaultFPU.Restriction != FPURestriction::SP_D16)
+  return false;// meaningless for this arch
+
+FPUKind = ARM::FK_INVALID;
+for (const FPUName  : FPUNames) {
+  if (CandidateFPU.FPUVer == DefaultFPU.FPUVer &&
+  CandidateFPU.NeonSupport == DefaultFPU.NeonSupport &&
+  CandidateFPU.Restriction == FPURestriction::D16) {
+FPUKind = CandidateFPU.ID;
+break;
+  }
+}
+  }
+}
+if (FPUKind == ARM::FK_INVALID)
+  return false;
+return ARM::getFPUFeatures(FPUKind, Features);
+  }
+
+  return false;
+}
+
 StringRef ARM::getHWDivName(unsigned HWDivKind) {
   for (const auto D : HWDivNames) {
 if (HWDivKind == D.ID)
Index: llvm/include/llvm/Support/ARMTargetParser.h
===
--- llvm/include/llvm/Support/ARMTargetParser.h
+++ llvm/include/llvm/Support/ARMTargetParser.h
@@ -233,6 +233,8 @@
 StringRef getSubArch(ArchKind AK);
 StringRef getArchExtName(unsigned ArchExtKind);
 StringRef getArchExtFeature(StringRef ArchExt);
+bool appendArchExtFeatures(StringRef CPU, ARM::ArchKind AK, StringRef ArchExt,
+   std::vector );
 StringRef getHWDivName(unsigned HWDivKind);
 
 // Information by Name
Index: clang/lib/Driver/ToolChains/Arch/ARM.h