[PATCH] D60472: [AArch64][PowerPC][Driver] Allow setting crypto feature through -mcrypto for ARM/AArch64

[Manoj Gupta via Phabricator via cfe-commits]

manojgupta added a comment.

In D60472#1461351 , @peter.smith wrote:

>




> Is that not a limitation of the build system? I'd expect a package to be able 
> to locally override a global default rather than vice-versa. Although crypto 
> might be the area of concern here and there may be a conveniently named 
> option for PPC, where does this stop? Crypto is not the only architectural 
> extension, for example see 
> https://gcc.gnu.org/onlinedocs/gcc/AArch64-Options.html . To maintain a 
> consistent interface we'd need command line options for all the extensions. 
> May I encourage you to reply to the RFC on command line options that I 
> mentioned earlier if it doesn't work for you? I think the extensions need to 
> be considered as a whole and not just individually.

I'll also read the RFC and respond.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60472



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

[PATCH] D60472: [AArch64][PowerPC][Driver] Allow setting crypto feature through -mcrypto for ARM/AArch64

[Manoj Gupta via Phabricator via cfe-commits]

manojgupta added a comment.

In D60472#1461351 , @peter.smith wrote:

>




> Is that not a limitation of the build system? I'd expect a package to be able 
> to locally override a global default rather than vice-versa. Although crypto 
> might be the area of concern here and there may be a conveniently named 
> option for PPC, where does this stop? Crypto is not the only architectural 
> extension, for example see 
> https://gcc.gnu.org/onlinedocs/gcc/AArch64-Options.html . To maintain a 
> consistent interface we'd need command line options for all the extensions. 
> May I encourage you to reply to the RFC on command line options that I 
> mentioned earlier if it doesn't work for you? I think the extensions need to 
> be considered as a whole and not just individually.

While it partly is a build system issue, another problem is enabling crypto via 
"-march" requires picking an architecture as well. So even if it could override 
the global default, it would also override the global "-march" as well. If the 
global "-march" was a better/higher option, it does not make sense to override 
it e.g. "-march=armv8-a+crypto" overriding "-march=armv8.3-a" is not helpful 
since it will disable 8.3 features.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60472



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

[PATCH] D60472: [AArch64][PowerPC][Driver] Allow setting crypto feature through -mcrypto for ARM/AArch64

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

In D60472#1461336 , @manojgupta wrote:

> The motivation for this change is to make "crypto" setting an additive option 
> e.g. like "-mavx" used in many media packages.  Some packages in Chrome want 
> to enable crypto conditionally for a few files to allow crypto feature to be 
> used based on runtime cpu detection. They set "-march=armv8+crypto" flag but 
> it gets overridden by the global "-march=armv8a" flag set by the build system 
> in Chrome OS because the target cpu does not support crypto causing 
> compile-time errors. 
>  Ability to specify "-mcrypto"  standalone makes it an additive option and 
> ensures that it it is not lost. i.e. this will help in decoupling  "-mcrypto" 
> from "-march" so that they could be set independently. The current additive 
> alternate is  '-Xclang -target-feature -Xclang "+crypto" ' which is ugly.


Is that not a limitation of the build system? I'd expect a package to be able 
to locally override a global default rather than vice-versa. Although crypto 
might be the area of concern here and there may be a conveniently named option 
for PPC, where does this stop? Crypto is not the only architectural extension, 
for example see https://gcc.gnu.org/onlinedocs/gcc/AArch64-Options.html . To 
maintain a consistent interface we'd need command line options for all the 
extensions. May I encourage you to reply to the RFC on command line options 
that I mentioned earlier if it doesn't work for you? I think the extensions 
need to be considered as a whole and not just individually.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60472



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

[PATCH] D60472: [AArch64][PowerPC][Driver] Allow setting crypto feature through -mcrypto for ARM/AArch64

[Manoj Gupta via Phabricator via cfe-commits]

manojgupta added a comment.

The motivation for this change is to make "crypto" setting an additive option 
e.g. like "-mavx" used in many media packages.  Some packages in Chrome want to 
enable crypto conditionally for a few files to allow crypto feature to be used 
based on runtime cpu detection. They set "-march=armv8+crypto" flag but it gets 
overridden by the global "-march=armv8a" flag set by the build system in Chrome 
OS because the target cpu does not support crypto causing compile-time errors. 
Ability to specify "-mcrypto"  standalone makes it an additive option and 
ensures that it it is not lost. i.e. this will help in decoupling  "-mcrypto" 
from "-march" so that they could be set independently. The current additive 
alternate is  '-Xclang -target-feature -Xclang "+crypto" ' which is ugly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60472



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

[PATCH] D60472: [AArch64][PowerPC][Driver] Allow setting crypto feature through -mcrypto for ARM/AArch64

[Peter Smith via Phabricator via cfe-commits]

peter.smith added a comment.

I'm not in favour of adding AArch64 support to -mcrypto -mnocrypto for a few 
reasons:

- Arm are trying to keep the options for controlling target features as 
consistent as possible with GCC and this option isn't supported in GCC 
(https://gcc.gnu.org/onlinedocs/gcc/AArch64-Options.html)
- There is http://lists.llvm.org/pipermail/llvm-dev/2018-September/126346.html 
which is Arm's proposal for how target options can be better supported in 
Clang. I think that supporting -mcrypto as well would add more complexity as 
there are interactions between the options.
- Arm 32-bit also supports crypto so this would need to be added to Arm as well 
for consistency.

Can you expand on why you need -m[no]crypto?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60472



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

[PATCH] D60472: [AArch64][PowerPC][Driver] Allow setting crypto feature through -mcrypto for ARM/AArch64

[David Spickett via Phabricator via cfe-commits]

DavidSpickett added a comment.

What's the motivation for this change, are you working towards common flags for 
both platforms? The current way to select crypto on AArch64 is 
'-march=armv8.x-a+crypto/nocrypto'. I can see that would be an issue if Power 
PC doesn't support that syntax, or doesn't have a specific crypto extension.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60472



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

[PATCH] D60472: [AArch64][PowerPC][Driver] Allow setting crypto feature through -mcrypto for ARM/AArch64

[Tiancong Wang via Phabricator via cfe-commits]

tcwang updated this revision to Diff 194371.
tcwang marked an inline comment as done.
tcwang added a comment.

Fix some comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60472

Files:
  clang/include/clang/Driver/Options.td
  clang/lib/Driver/ToolChains/Arch/AArch64.cpp
  clang/lib/Driver/ToolChains/Arch/ARM.cpp
  clang/lib/Driver/ToolChains/Arch/PPC.cpp
  clang/lib/Driver/ToolChains/Arch/PPC.h
  clang/test/Driver/aarch64-crypto.c
  clang/test/Driver/armv8-crypto.c

Index: clang/test/Driver/armv8-crypto.c
===
--- /dev/null
+++ clang/test/Driver/armv8-crypto.c
@@ -0,0 +1,18 @@
+// REQUIRES: arm-registered-target
+// RUN: %clang -target armv8 -mcrypto -mhard-float -### %s 2> %t
+// RUN: FileCheck --check-prefix=CHECK-V8-CRYPTO < %t %s
+// CHECK-V8-CRYPTO: "-target-feature" "+crypto"
+
+// RUN: %clang -target armv8 -mno-crypto -mhard-float -### %s 2> %t
+// RUN: FileCheck --check-prefix=CHECK-V8-NOCRYPTO < %t %s
+// CHECK-V8-NOCRYPTO: "-target-feature" "-crypto"
+
+// RUN: %clang -target armv8 -mcrypto -msoft-float -### %s 2> %t
+// RUN: FileCheck --check-prefix=CHECK-V8-CRYPTO-BEFORE-SOFT-FLOAT < %t %s
+// CHECK-V8-CRYPTO-BEFORE-SOFT-FLOAT: "-target-feature" "-crypto"
+// CHECK-V8-CRYPTO-BEFORE-SOFT-FLOAT-NOT: "-target-feature" "+crypto"
+
+// RUN: %clang -target armv8 -msoft-float -mcrypto -### %s 2> %t
+// RUN: FileCheck --check-prefix=CHECK-V8-CRYPTO-AFTER-SOFT-FLOAT < %t %s
+// CHECK-V8-CRYPTO-AFTER-SOFT-FLOAT: "-target-feature" "-crypto"
+// CHECK-V8-CRYPTO-AFTER-SOFT-FLOAT-NOT: "-target-feature" "+crypto"
Index: clang/test/Driver/aarch64-crypto.c
===
--- /dev/null
+++ clang/test/Driver/aarch64-crypto.c
@@ -0,0 +1,8 @@
+// REQUIRES: aarch64-registered-target
+// RUN: %clang -target aarch64 -mcrypto -### %s 2> %t
+// RUN: FileCheck --check-prefix=CHECK-AARCH64-CRYPTO < %t %s
+// CHECK-AARCH64-CRYPTO: "-target-feature" "+crypto"
+
+// RUN: %clang -target aarch64 -mno-crypto -### %s 2> %t
+// RUN: FileCheck --check-prefix=CHECK-AARCH64-NOCRYPTO < %t %s
+// CHECK-AARCH64-NOCRYPTO: "-target-feature" "-crypto"
Index: clang/lib/Driver/ToolChains/Arch/PPC.h
===
--- clang/lib/Driver/ToolChains/Arch/PPC.h
+++ clang/lib/Driver/ToolChains/Arch/PPC.h
@@ -22,6 +22,8 @@
 
 bool hasPPCAbiArg(const llvm::opt::ArgList , const char *Value);
 
+bool hasCryptoFeatureEnabled(const llvm::opt::ArgList );
+
 enum class FloatABI {
   Invalid,
   Soft,
Index: clang/lib/Driver/ToolChains/Arch/PPC.cpp
===
--- clang/lib/Driver/ToolChains/Arch/PPC.cpp
+++ clang/lib/Driver/ToolChains/Arch/PPC.cpp
@@ -109,6 +109,9 @@
   ppc::ReadGOTPtrMode ReadGOT = ppc::getPPCReadGOTPtrMode(D, Triple, Args);
   if (ReadGOT == ppc::ReadGOTPtrMode::SecurePlt)
 Features.push_back("+secure-plt");
+
+  if (ppc::hasCryptoFeatureEnabled(Args))
+Features.push_back("+crypto");
 }
 
 ppc::ReadGOTPtrMode ppc::getPPCReadGOTPtrMode(const Driver , const llvm::Triple ,
@@ -154,3 +157,10 @@
   Arg *A = Args.getLastArg(options::OPT_mabi_EQ);
   return A && (A->getValue() == StringRef(Value));
 }
+
+bool ppc::hasCryptoFeatureEnabled(const ArgList ) {
+  if (Arg *A = Args.getLastArg(options::OPT_mcrypto, options::OPT_mnocrypto))
+if (A->getOption().matches(options::OPT_mcrypto))
+  return true;
+  return false;
+}
Index: clang/lib/Driver/ToolChains/Arch/ARM.cpp
===
--- clang/lib/Driver/ToolChains/Arch/ARM.cpp
+++ clang/lib/Driver/ToolChains/Arch/ARM.cpp
@@ -448,6 +448,14 @@
   Features.push_back("-crc");
   }
 
+  // En/disable crypto
+  if (Arg *A = Args.getLastArg(options::OPT_mcrypto, options::OPT_mnocrypto)) {
+if (A->getOption().matches(options::OPT_mcrypto) && ABI != arm::FloatABI::Soft)
+  Features.push_back("+crypto");
+else
+  Features.push_back("-crypto");
+  }
+
   // For Arch >= ARMv8.0:  crypto = sha2 + aes
   // FIXME: this needs reimplementation after the TargetParser rewrite
   if (ArchName.find_lower("armv8a") != StringRef::npos ||
Index: clang/lib/Driver/ToolChains/Arch/AArch64.cpp
===
--- clang/lib/Driver/ToolChains/Arch/AArch64.cpp
+++ clang/lib/Driver/ToolChains/Arch/AArch64.cpp
@@ -188,6 +188,15 @@
   if (!success)
 D.Diag(diag::err_drv_clang_unsupported) << A->getAsString(Args);
 
+  // En/disable crypto
+  if (Arg *A = Args.getLastArg(options::OPT_mcrypto, options::OPT_mnocrypto,
+   options::OPT_mgeneral_regs_only)) {
+if (A->getOption().matches(options::OPT_mcrypto))
+  Features.push_back("+crypto");
+else
+  Features.push_back("-crypto");
+  }
+
   if 

[PATCH] D60472: [AArch64][PowerPC][Driver] Allow setting crypto feature through -mcrypto for ARM/AArch64

[Tiancong Wang via Phabricator via cfe-commits]

tcwang marked 4 inline comments as done.
tcwang added inline comments.



Comment at: clang/lib/Driver/ToolChains/Arch/AArch64.cpp:192
+  // En/disable crypto
+  if (Arg *A = Args.getLastArg(options::OPT_mcrypto, options::OPT_mnocrypto,
+   options::OPT_mgeneral_regs_only)) {

manojgupta wrote:
> I believe this should be merged with the code for OPT_mgeneral_regs_only 
> otherwise  the next if statement  for mgeneral-regs-only  would force 
> "-crypto" .
> 
> if (A->getOption().matches(OPT_mgeneral_regs_only)))
> ..// disable crypto, neon etc.
> else if (A->getOption().matches(options::OPT_mcrypto))
> // enable crypto
> ...
> 
> Please also add tests when mgeneral-regs-only is specified with "-mcrypto"  
> before/after.
Understand. My question of the logic is that if -mgeneral-regs-only comes 
before -mcrypto, do we want to set "-fp-armv8" "-neon" or not? In other words, 
if -mcrypto comes after -mgeneral-regs-only, does it only override '+crypto' or 
override all the three features? I'll change the logic and add tests after 
making sure what we really want to do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60472



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

[PATCH] D60472: [AArch64][PowerPC][Driver] Allow setting crypto feature through -mcrypto for ARM/AArch64

[Manoj Gupta via Phabricator via cfe-commits]

manojgupta added inline comments.



Comment at: clang/include/clang/Driver/Options.td:2218
 Group;
-def mpower8_crypto : Flag<["-"], "mcrypto">,
-Group;
-def mnopower8_crypto : Flag<["-"], "mno-crypto">,
-Group;
+def mcrypto : Flag<["-"], "mcrypto">, Group,
+HelpText<"Add use of cryptographic instructions (ARM/PowerPC only)">;

Can you move it out of ppc specific options area to more generic options 
location e.g. like hard-float?



Comment at: clang/include/clang/Driver/Options.td:2219
+def mcrypto : Flag<["-"], "mcrypto">, Group,
+HelpText<"Add use of cryptographic instructions (ARM/PowerPC only)">;
+def mnocrypto : Flag<["-"], "mno-crypto">, Group,

ARM/AArch64/PowerPC



Comment at: clang/include/clang/Driver/Options.td:2221
+def mnocrypto : Flag<["-"], "mno-crypto">, Group,
+HelpText<"Disallow use of cryptographic instructions (ARM/PowerPC only)">;
 def mdirect_move : Flag<["-"], "mdirect-move">,

ARM/AArch64/PowerPC



Comment at: clang/lib/Driver/ToolChains/Arch/AArch64.cpp:192
+  // En/disable crypto
+  if (Arg *A = Args.getLastArg(options::OPT_mcrypto, options::OPT_mnocrypto,
+   options::OPT_mgeneral_regs_only)) {

I believe this should be merged with the code for OPT_mgeneral_regs_only 
otherwise  the next if statement  for mgeneral-regs-only  would force "-crypto" 
.

if (A->getOption().matches(OPT_mgeneral_regs_only)))
..// disable crypto, neon etc.
else if (A->getOption().matches(options::OPT_mcrypto))
// enable crypto
...

Please also add tests when mgeneral-regs-only is specified with "-mcrypto"  
before/after.



Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:453
+  if (Arg *A = Args.getLastArg(options::OPT_mcrypto, options::OPT_mnocrypto)) {
+if (A->getOption().matches(options::OPT_mcrypto) && ABI != 
arm::FloatABI::Soft)
+  Features.push_back("+crypto");

Please add a test for interaction with soft-float ABI option.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D60472



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