Re: [PATCH] D20088: Using AArch64TargetParser in clang

2016-05-25 Thread Renato Golin via cfe-commits 
rengolin closed this revision.
rengolin added a comment.

Committed in r270688.


Repository:
  rL LLVM

http://reviews.llvm.org/D20088



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20088: Using AArch64TargetParser in clang

2016-05-25 Thread Renato Golin via cfe-commits 
rengolin accepted this revision.
rengolin added a comment.
This revision is now accepted and ready to land.

Looks great, thanks Jojo!


Repository:
  rL LLVM

http://reviews.llvm.org/D20088



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20088: Using AArch64TargetParser in clang

2016-05-24 Thread jojo.ma via cfe-commits 
jojo set the repository for this revision to rL LLVM.
jojo changed the visibility of this Differential Revision from "Public (No 
Login Required)" to "All Users".
jojo updated this revision to Diff 58210.
jojo added a comment.

Adjust "getAArch64ArchFeaturesFromMarch" logic.In file lib/Driver/Tools.cpp.


Repository:
  rL LLVM

http://reviews.llvm.org/D20088

Files:
  lib/Basic/Targets.cpp
  lib/Driver/Tools.cpp

Index: lib/Driver/Tools.cpp
===
--- lib/Driver/Tools.cpp
+++ lib/Driver/Tools.cpp
@@ -2223,22 +2223,9 @@
   text.split(Split, StringRef("+"), -1, false);
 
   for (StringRef Feature : Split) {
-const char *result = llvm::StringSwitch(Feature)
- .Case("fp", "+fp-armv8")
- .Case("simd", "+neon")
- .Case("crc", "+crc")
- .Case("crypto", "+crypto")
- .Case("fp16", "+fullfp16")
- .Case("profile", "+spe")
- .Case("nofp", "-fp-armv8")
- .Case("nosimd", "-neon")
- .Case("nocrc", "-crc")
- .Case("nocrypto", "-crypto")
- .Case("nofp16", "-fullfp16")
- .Case("noprofile", "-spe")
- .Default(nullptr);
-if (result)
-  Features.push_back(result);
+const char *FeatureName = llvm::AArch64::getArchExtFeature(Feature);
+if (FeatureName)
+  Features.push_back(FeatureName);
 else if (Feature == "neon" || Feature == "noneon")
   D.Diag(diag::err_drv_no_neon_modifier);
 else
@@ -2253,20 +2240,16 @@
   std::vector ) {
   std::pair Split = Mcpu.split("+");
   CPU = Split.first;
-  if (CPU == "cortex-a53" || CPU == "cortex-a57" ||
-  CPU == "cortex-a72" || CPU == "cortex-a35" || CPU == "exynos-m1" ||
-  CPU == "kryo") {
-Features.push_back("+neon");
-Features.push_back("+crc");
-Features.push_back("+crypto");
-  } else if (CPU == "cyclone") {
-Features.push_back("+neon");
-Features.push_back("+crypto");
-  } else if (CPU == "generic") {
+
+  if (CPU == "generic") {
 Features.push_back("+neon");
   } else {
-return false;
-  }
+unsigned ArchKind = llvm::AArch64::parseCPUArch(CPU);
+unsigned Extersion = llvm::AArch64::getDefaultExtensions(CPU,ArchKind);
+
+if (!llvm::AArch64::getExtensionFeatures(Extersion,Features))
+  return false;
+   }
 
   if (Split.second.size() && !DecodeAArch64Features(D, Split.second, Features))
 return false;
@@ -2278,20 +2261,13 @@
 getAArch64ArchFeaturesFromMarch(const Driver , StringRef March,
 const ArgList ,
 std::vector ) {
+  unsigned ArchKind;
   std::string MarchLowerCase = March.lower();
   std::pair Split = StringRef(MarchLowerCase).split("+");
 
-  if (Split.first == "armv8-a" || Split.first == "armv8a") {
-// ok, no additional features.
-  } else if (Split.first == "armv8.1-a" || Split.first == "armv8.1a") {
-Features.push_back("+v8.1a");
-  } else if (Split.first == "armv8.2-a" || Split.first == "armv8.2a" ) {
-Features.push_back("+v8.2a");
-  } else {
-return false;
-  }
-
-  if (Split.second.size() && !DecodeAArch64Features(D, Split.second, Features))
+  ArchKind = llvm::AArch64::parseArch(Split.first);
+  if (ArchKind == llvm::ARM::AK_INVALID || !llvm::AArch64::getArchFeatures(ArchKind, Features) ||
+	  (Split.second.size() && !DecodeAArch64Features(D, Split.second, Features)))
 return false;
 
   return true;
Index: lib/Basic/Targets.cpp
===
--- lib/Basic/Targets.cpp
+++ lib/Basic/Targets.cpp
@@ -5665,14 +5665,10 @@
   }
 
   bool setCPU(const std::string ) override {
-bool CPUKnown = llvm::StringSwitch(Name)
-.Case("generic", true)
-.Cases("cortex-a53", "cortex-a57", "cortex-a72",
-   "cortex-a35", "exynos-m1", true)
-.Case("cyclone", true)
-.Case("kryo", true)
-.Default(false);
-return CPUKnown;
+if (Name == "generic" || llvm::AArch64::parseCPUArch(Name) != llvm::ARM::AK_INVALID)
+  return true;
+
+return false;
   }
 
   void getTargetDefines(const LangOptions ,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20088: Using AArch64TargetParser in clang

2016-05-23 Thread jojo.ma via cfe-commits 
jojo removed rL LLVM as the repository for this revision.
jojo changed the visibility of this Differential Revision from "All Users" to 
"Public (No Login Required)".
jojo updated this revision to Diff 58072.
jojo added a comment.

Remove checkARMArchValid & checkAArch64ArchValid logic.


http://reviews.llvm.org/D20088

Files:
  lib/Basic/Targets.cpp
  lib/Driver/Tools.cpp

Index: lib/Driver/Tools.cpp
===
--- lib/Driver/Tools.cpp
+++ lib/Driver/Tools.cpp
@@ -,22 +,9 @@
   text.split(Split, StringRef("+"), -1, false);
 
   for (StringRef Feature : Split) {
-const char *result = llvm::StringSwitch(Feature)
- .Case("fp", "+fp-armv8")
- .Case("simd", "+neon")
- .Case("crc", "+crc")
- .Case("crypto", "+crypto")
- .Case("fp16", "+fullfp16")
- .Case("profile", "+spe")
- .Case("nofp", "-fp-armv8")
- .Case("nosimd", "-neon")
- .Case("nocrc", "-crc")
- .Case("nocrypto", "-crypto")
- .Case("nofp16", "-fullfp16")
- .Case("noprofile", "-spe")
- .Default(nullptr);
-if (result)
-  Features.push_back(result);
+const char *FeatureName = llvm::AArch64::getArchExtFeature(Feature);
+if (FeatureName)
+  Features.push_back(FeatureName);
 else if (Feature == "neon" || Feature == "noneon")
   D.Diag(diag::err_drv_no_neon_modifier);
 else
@@ -2252,20 +2239,16 @@
   std::vector ) {
   std::pair Split = Mcpu.split("+");
   CPU = Split.first;
-  if (CPU == "cortex-a53" || CPU == "cortex-a57" ||
-  CPU == "cortex-a72" || CPU == "cortex-a35" || CPU == "exynos-m1" ||
-  CPU == "kryo") {
-Features.push_back("+neon");
-Features.push_back("+crc");
-Features.push_back("+crypto");
-  } else if (CPU == "cyclone") {
-Features.push_back("+neon");
-Features.push_back("+crypto");
-  } else if (CPU == "generic") {
+
+  if (CPU == "generic") {
 Features.push_back("+neon");
   } else {
-return false;
-  }
+unsigned ArchKind = llvm::AArch64::parseCPUArch(CPU);
+unsigned Extersion = llvm::AArch64::getDefaultExtensions(CPU,ArchKind);
+
+if (!llvm::AArch64::getExtensionFeatures(Extersion,Features))
+  return false;
+   }
 
   if (Split.second.size() && !DecodeAArch64Features(D, Split.second, Features))
 return false;
@@ -2280,17 +2263,8 @@
   std::string MarchLowerCase = March.lower();
   std::pair Split = StringRef(MarchLowerCase).split("+");
 
-  if (Split.first == "armv8-a" || Split.first == "armv8a") {
-// ok, no additional features.
-  } else if (Split.first == "armv8.1-a" || Split.first == "armv8.1a") {
-Features.push_back("+v8.1a");
-  } else if (Split.first == "armv8.2-a" || Split.first == "armv8.2a" ) {
-Features.push_back("+v8.2a");
-  } else {
-return false;
-  }
-
-  if (Split.second.size() && !DecodeAArch64Features(D, Split.second, Features))
+  if (llvm::AArch64::parseArch(Split.first, Features) == llvm::ARM::AK_INVALID ||
+	  (Split.second.size() && !DecodeAArch64Features(D, Split.second, Features)))
 return false;
 
   return true;
Index: lib/Basic/Targets.cpp
===
--- lib/Basic/Targets.cpp
+++ lib/Basic/Targets.cpp
@@ -5647,14 +5647,10 @@
   }
 
   bool setCPU(const std::string ) override {
-bool CPUKnown = llvm::StringSwitch(Name)
-.Case("generic", true)
-.Cases("cortex-a53", "cortex-a57", "cortex-a72",
-   "cortex-a35", "exynos-m1", true)
-.Case("cyclone", true)
-.Case("kryo", true)
-.Default(false);
-return CPUKnown;
+if (Name == "generic" || llvm::AArch64::parseCPUArch(Name) != llvm::ARM::AK_INVALID)
+  return true;
+
+return false;
   }
 
   void getTargetDefines(const LangOptions ,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20088: Using AArch64TargetParser in clang

2016-05-23 Thread jojo.ma via cfe-commits 
jojo added inline comments.


Comment at: lib/Driver/Tools.cpp:707
@@ -696,3 +706,3 @@
   std::string MArch = arm::getARMArch(ArchName, Triple);
-  if (llvm::ARM::parseArch(MArch) == llvm::ARM::AK_INVALID ||
+  if (!checkARMArchValid(MArch) || llvm::ARM::parseArch(MArch) == 
llvm::ARM::AK_INVALID ||
   (Split.second.size() && !DecodeARMFeatures(D, Split.second, Features)))

bsmith wrote:
> Why do we need the call to checkARMArchValid here? Isn't is sufficient that 
> parseArch returns a valid architecture?
Let the TargetParser return correct result.Remove this in the next patchset.


Comment at: lib/Driver/Tools.cpp:2280
@@ -2276,12 +2279,3 @@
 
-  if (Split.first == "armv8-a" || Split.first == "armv8a") {
-// ok, no additional features.
-  } else if (Split.first == "armv8.1-a" || Split.first == "armv8.1a") {
-Features.push_back("+v8.1a");
-  } else if (Split.first == "armv8.2-a" || Split.first == "armv8.2a" ) {
-Features.push_back("+v8.2a");
-  } else {
-return false;
-  }
-
-  if (Split.second.size() && !DecodeAArch64Features(D, Split.second, Features))
+  if (!checkAArch64ArchValid(Split.first) || 
llvm::AArch64::parseArch(Split.first, Features) == llvm::ARM::AK_INVALID ||
+ (Split.second.size() && !DecodeAArch64Features(D, Split.second, 
Features)))

bsmith wrote:
> Same here, why do we need checkAArch64ArchValid?
Same as above.


Repository:
  rL LLVM

http://reviews.llvm.org/D20088



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20088: Using AArch64TargetParser in clang

2016-05-10 Thread Renato Golin via cfe-commits 
rengolin added a comment.

In http://reviews.llvm.org/D20088#425589, @rengolin wrote:

> In this case, it may be simpler to just rely on the Triple object that 
> already exists in the driver, or just the fact that this is in 
> getAArch64ArchFeaturesFromMarch() and pass an extra flag to parseArch() 
> forcing it to only recognise 32-bit or 64-bit arches as valid.


Or, better still, when this is class-based and has inheritance, the 
AArch64TargetParser will only be used for 64-bit targets, so it'll know already 
what to refuse (ie. anything before v8).


Repository:
  rL LLVM

http://reviews.llvm.org/D20088



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20088: Using AArch64TargetParser in clang

2016-05-10 Thread Renato Golin via cfe-commits 
rengolin added a comment.

Bradley, Jojo,

I'm a bit rusty in that part of the code, but that's not really what I was 
thinking either.

The way we started this was to have the TargetParser have *all* parsing. So 
both check*ArchValid, if necessary, will have to use the TargetParser to parse 
the strings, and return valid or not. The best way to do this, so far, has been 
to just parse the Arch and return INVALID, as Bradley said.

However, the uncertainty around AArch64/AArch32 vs ARMv8/ARMv8.1 (and possible 
future v9, v10...) does create an additional layer of complexity, which I, 
admittedly, haven't grasped fully.

In this case, it may be simpler to just rely on the Triple object that already 
exists in the driver, or just the fact that this is in 
getAArch64ArchFeaturesFromMarch() and pass an extra flag to parseArch() forcing 
it to only recognise 32-bit or 64-bit arches as valid.

cheers,
--renato


Repository:
  rL LLVM

http://reviews.llvm.org/D20088



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D20088: Using AArch64TargetParser in clang

2016-05-10 Thread Bradley Smith via cfe-commits 
bsmith added inline comments.


Comment at: lib/Driver/Tools.cpp:707
@@ -696,3 +706,3 @@
   std::string MArch = arm::getARMArch(ArchName, Triple);
-  if (llvm::ARM::parseArch(MArch) == llvm::ARM::AK_INVALID ||
+  if (!checkARMArchValid(MArch) || llvm::ARM::parseArch(MArch) == 
llvm::ARM::AK_INVALID ||
   (Split.second.size() && !DecodeARMFeatures(D, Split.second, Features)))

Why do we need the call to checkARMArchValid here? Isn't is sufficient that 
parseArch returns a valid architecture?


Comment at: lib/Driver/Tools.cpp:2280
@@ -2276,12 +2279,3 @@
 
-  if (Split.first == "armv8-a" || Split.first == "armv8a") {
-// ok, no additional features.
-  } else if (Split.first == "armv8.1-a" || Split.first == "armv8.1a") {
-Features.push_back("+v8.1a");
-  } else if (Split.first == "armv8.2-a" || Split.first == "armv8.2a" ) {
-Features.push_back("+v8.2a");
-  } else {
-return false;
-  }
-
-  if (Split.second.size() && !DecodeAArch64Features(D, Split.second, Features))
+  if (!checkAArch64ArchValid(Split.first) || 
llvm::AArch64::parseArch(Split.first, Features) == llvm::ARM::AK_INVALID ||
+ (Split.second.size() && !DecodeAArch64Features(D, Split.second, 
Features)))

Same here, why do we need checkAArch64ArchValid?


Repository:
  rL LLVM

http://reviews.llvm.org/D20088



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits