[PATCH] D50179: [AArch64][ARM] Context sensitive meaning of option "crypto"

[Eli Friedman via Phabricator via cfe-commits]

efriedma added inline comments.


================
Comment at: lib/Driver/ToolChains/Arch/AArch64.cpp:200
+  //
+  // TODO: implement this logic in TargetParser.
+
----------------
Yes, this logic should be in TargetParser, not here.  Trying to rewrite the target features afterwards is messy at best.  (Actually, the target feature list generated by TargetParser probably shouldn't contain the string "crypto" at all.)

Given that this code will go away when TargetParser is fixed, why are you proposing this patch?


================
Comment at: lib/Driver/ToolChains/Arch/AArch64.cpp:219
+
+  if (std::find(ItBegin, ItEnd, "+v8.4a") != ItEnd) {
+    if (HasCrypto && !NoCrypto) {
----------------
Does this need to check for 8.5a?


================
Comment at: lib/Driver/ToolChains/Arch/ARM.cpp:430
+      if (ArchName.find_lower("+noaes") == StringRef::npos)
+        Features.push_back("+aes");
+    } else if (ArchName.find_lower("-crypto") != StringRef::npos) {
----------------
The ARM backend doesn't support features named "sha2" and "aes" at the moment.


https://reviews.llvm.org/D50179