subject:"\[PATCH\] D67608\: \[ARM\] Preserve fpu behaviour for '\-crypto'"

[PATCH] D67608: [ARM] Preserve fpu behaviour for '-crypto'

2019-10-14 Thread Diogo N. Sampaio via Phabricator via cfe-commits

This revision was automatically updated to reflect the committed changes.
Closed by commit rG2cb43b45713d: [ARM] Preserve fpu behaviour for 
-crypto (authored by dnsampaio).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67608

Files:
  clang/lib/Driver/ToolChains/Arch/ARM.cpp
  clang/test/Driver/arm-features.c


Index: clang/test/Driver/arm-features.c
===
--- clang/test/Driver/arm-features.c
+++ clang/test/Driver/arm-features.c
@@ -79,3 +79,18 @@
 // RUN: %clang -target arm-arm-none-eabi -mcpu=cortex-m23+crypto -### -c %s 
2>&1 | FileCheck -check-prefix=CHECK-NOCRYPTO5 %s
 // CHECK-NOCRYPTO5: warning: ignoring extension 'crypto' because the {{.*}} 
architecture does not support it
 // CHECK-NOCRYPTO5-NOT: "-target-feature" "+crypto"{{.*}} "-target-feature" 
"+sha2" "-target-feature" "+aes"
+//
+// Check +crypto does not affect -march=armv7a -mfpu=crypto-neon-fp-armv8, but 
it does warn that +crypto has no effect
+// RUN: %clang -target arm-none-none-eabi -fno-integrated-as -march=armv7a 
-mfpu=crypto-neon-fp-armv8 -### -c %s 2>&1 | FileCheck 
-check-prefixes=CHECK-WARNONLY,ALL %s
+// RUN: %clang -target arm-none-none-eabi -fno-integrated-as -march=armv7a+aes 
-mfpu=crypto-neon-fp-armv8 -### -c %s 2>&1 | FileCheck 
-check-prefixes=CHECK-WARNONLY,ALL,CHECK-HASAES %s
+// RUN: %clang -target arm-none-none-eabi -fno-integrated-as 
-march=armv7a+sha2 -mfpu=crypto-neon-fp-armv8 -### -c %s 2>&1 | FileCheck 
-check-prefixes=CHECK-WARNONLY,ALL,CHECK-HASSHA %s
+// RUN: %clang -target arm-none-none-eabi -fno-integrated-as 
-march=armv7a+sha2+aes -mfpu=crypto-neon-fp-armv8 -### -c %s 2>&1 | FileCheck 
-check-prefixes=CHECK-WARNONLY,ALL,CHECK-HASSHA,CHECK-HASAES %s
+// RUN: %clang -target arm-none-none-eabi -fno-integrated-as -march=armv7a+aes 
-mfpu=neon-fp-armv8 -### -c %s 2>&1 | FileCheck 
-check-prefixes=ALL,CHECK-HASAES %s
+// RUN: %clang -target arm-none-none-eabi -fno-integrated-as 
-march=armv7a+sha2 -mfpu=neon-fp-armv8 -### -c %s 2>&1 | FileCheck 
-check-prefixes=ALL,CHECK-HASSHA %s
+// RUN: %clang -target arm-none-none-eabi -fno-integrated-as 
-march=armv7a+sha2+aes -mfpu=neon-fp-armv8 -### -c %s 2>&1 | FileCheck 
-check-prefixes=ALL,CHECK-HASSHA,CHECK-HASAES %s
+// CHECK-WARNONLY: warning: ignoring extension 'crypto' because the 'armv7-a' 
architecture does not support it
+// ALL: "-target-feature"
+// CHECK-WARNONLY-NOT: "-target-feature" "-crypto"
+// CHECK-HASSHA-SAME:  "-target-feature" "+sha2"
+// CHECK-HASAES-SAME:  "-target-feature" "+aes"
+//
Index: clang/lib/Driver/ToolChains/Arch/ARM.cpp
===
--- clang/lib/Driver/ToolChains/Arch/ARM.cpp
+++ clang/lib/Driver/ToolChains/Arch/ARM.cpp
@@ -479,24 +479,33 @@
 
   // For Arch >= ARMv8.0 && A profile:  crypto = sha2 + aes
   // FIXME: this needs reimplementation after the TargetParser rewrite
-  auto CryptoIt =
-llvm::find_if(llvm::reverse(Features),
-  [](const StringRef F) { return F.contains("crypto"); });
-  if (CryptoIt != Features.rend() && CryptoIt->take_front() == "+") {
-StringRef ArchSuffix = arm::getLLVMArchSuffixForARM(
-arm::getARMTargetCPU(CPUName, ArchName, Triple), ArchName, Triple);
-if (llvm::ARM::parseArchVersion(ArchSuffix) >= 8 &&
-llvm::ARM::parseArchProfile(ArchSuffix) == llvm::ARM::ProfileKind::A) {
-  if (ArchName.find_lower("+nosha2") == StringRef::npos &&
-  CPUName.find_lower("+nosha2") == StringRef::npos)
-Features.push_back("+sha2");
-  if (ArchName.find_lower("+noaes") == StringRef::npos &&
-  CPUName.find_lower("+noaes") == StringRef::npos)
-Features.push_back("+aes");
-} else {
-  D.Diag(clang::diag::warn_target_unsupported_extension)
-<< "crypto" << 
llvm::ARM::getArchName(llvm::ARM::parseArch(ArchSuffix));
-  Features.push_back("-crypto");
+  auto CryptoIt = llvm::find_if(llvm::reverse(Features), [](const StringRef F) 
{
+return F.contains("crypto");
+  });
+  if (CryptoIt != Features.rend()) {
+if (CryptoIt->take_front() == "+") {
+  StringRef ArchSuffix = arm::getLLVMArchSuffixForARM(
+  arm::getARMTargetCPU(CPUName, ArchName, Triple), ArchName, Triple);
+  if (llvm::ARM::parseArchVersion(ArchSuffix) >= 8 &&
+  llvm::ARM::parseArchProfile(ArchSuffix) ==
+  llvm::ARM::ProfileKind::A) {
+if (ArchName.find_lower("+nosha2") == StringRef::npos &&
+CPUName.find_lower("+nosha2") == StringRef::npos)
+  Features.push_back("+sha2");
+if (ArchName.find_lower("+noaes") == StringRef::npos &&
+CPUName.find_lower("+noaes") == StringRef::npos)
+  Features.push_back("+aes");
+  } else {
+D.Diag(clang::diag::warn_target_unsupported_extension)
+<< "crypto"
+<<

[PATCH] D67608: [ARM] Preserve fpu behaviour for '-crypto'

2019-10-10 Thread Peter Smith via Phabricator via cfe-commits

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

Thanks for the update. That looks good to me. Will be good to wait for a day 
before committing to give US timezone a chance to object.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67608



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

[PATCH] D67608: [ARM] Preserve fpu behaviour for '-crypto'

2019-10-10 Thread Diogo N. Sampaio via Phabricator via cfe-commits

dnsampaio updated this revision to Diff 224323.
dnsampaio added a comment.

Attending review request:

- Fixed to only add '-crypto' when not passing -fno-integrated-as
- Fixed test to use arm-none-none-eabi
- Changed comments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67608

Files:
  clang/lib/Driver/ToolChains/Arch/ARM.cpp
  clang/test/Driver/arm-features.c


Index: clang/test/Driver/arm-features.c
===
--- clang/test/Driver/arm-features.c
+++ clang/test/Driver/arm-features.c
@@ -79,3 +79,18 @@
 // RUN: %clang -target arm-arm-none-eabi -mcpu=cortex-m23+crypto -### -c %s 
2>&1 | FileCheck -check-prefix=CHECK-NOCRYPTO5 %s
 // CHECK-NOCRYPTO5: warning: ignoring extension 'crypto' because the {{.*}} 
architecture does not support it
 // CHECK-NOCRYPTO5-NOT: "-target-feature" "+crypto"{{.*}} "-target-feature" 
"+sha2" "-target-feature" "+aes"
+//
+// Check +crypto does not affect -march=armv7a -mfpu=crypto-neon-fp-armv8, but 
it does warn that +crypto has no effect
+// RUN: %clang -target arm-none-none-eabi -fno-integrated-as -march=armv7a 
-mfpu=crypto-neon-fp-armv8 -### -c %s 2>&1 | FileCheck 
-check-prefixes=CHECK-WARNONLY,ALL %s
+// RUN: %clang -target arm-none-none-eabi -fno-integrated-as -march=armv7a+aes 
-mfpu=crypto-neon-fp-armv8 -### -c %s 2>&1 | FileCheck 
-check-prefixes=CHECK-WARNONLY,ALL,CHECK-HASAES %s
+// RUN: %clang -target arm-none-none-eabi -fno-integrated-as 
-march=armv7a+sha2 -mfpu=crypto-neon-fp-armv8 -### -c %s 2>&1 | FileCheck 
-check-prefixes=CHECK-WARNONLY,ALL,CHECK-HASSHA %s
+// RUN: %clang -target arm-none-none-eabi -fno-integrated-as 
-march=armv7a+sha2+aes -mfpu=crypto-neon-fp-armv8 -### -c %s 2>&1 | FileCheck 
-check-prefixes=CHECK-WARNONLY,ALL,CHECK-HASSHA,CHECK-HASAES %s
+// RUN: %clang -target arm-none-none-eabi -fno-integrated-as -march=armv7a+aes 
-mfpu=neon-fp-armv8 -### -c %s 2>&1 | FileCheck 
-check-prefixes=ALL,CHECK-HASAES %s
+// RUN: %clang -target arm-none-none-eabi -fno-integrated-as 
-march=armv7a+sha2 -mfpu=neon-fp-armv8 -### -c %s 2>&1 | FileCheck 
-check-prefixes=ALL,CHECK-HASSHA %s
+// RUN: %clang -target arm-none-none-eabi -fno-integrated-as 
-march=armv7a+sha2+aes -mfpu=neon-fp-armv8 -### -c %s 2>&1 | FileCheck 
-check-prefixes=ALL,CHECK-HASSHA,CHECK-HASAES %s
+// CHECK-WARNONLY: warning: ignoring extension 'crypto' because the 'armv7-a' 
architecture does not support it
+// ALL: "-target-feature"
+// CHECK-WARNONLY-NOT: "-target-feature" "-crypto"
+// CHECK-HASSHA-SAME:  "-target-feature" "+sha2"
+// CHECK-HASAES-SAME:  "-target-feature" "+aes"
+//
Index: clang/lib/Driver/ToolChains/Arch/ARM.cpp
===
--- clang/lib/Driver/ToolChains/Arch/ARM.cpp
+++ clang/lib/Driver/ToolChains/Arch/ARM.cpp
@@ -479,24 +479,33 @@
 
   // For Arch >= ARMv8.0 && A profile:  crypto = sha2 + aes
   // FIXME: this needs reimplementation after the TargetParser rewrite
-  auto CryptoIt =
-llvm::find_if(llvm::reverse(Features),
-  [](const StringRef F) { return F.contains("crypto"); });
-  if (CryptoIt != Features.rend() && CryptoIt->take_front() == "+") {
-StringRef ArchSuffix = arm::getLLVMArchSuffixForARM(
-arm::getARMTargetCPU(CPUName, ArchName, Triple), ArchName, Triple);
-if (llvm::ARM::parseArchVersion(ArchSuffix) >= 8 &&
-llvm::ARM::parseArchProfile(ArchSuffix) == llvm::ARM::ProfileKind::A) {
-  if (ArchName.find_lower("+nosha2") == StringRef::npos &&
-  CPUName.find_lower("+nosha2") == StringRef::npos)
-Features.push_back("+sha2");
-  if (ArchName.find_lower("+noaes") == StringRef::npos &&
-  CPUName.find_lower("+noaes") == StringRef::npos)
-Features.push_back("+aes");
-} else {
-  D.Diag(clang::diag::warn_target_unsupported_extension)
-<< "crypto" << 
llvm::ARM::getArchName(llvm::ARM::parseArch(ArchSuffix));
-  Features.push_back("-crypto");
+  auto CryptoIt = llvm::find_if(llvm::reverse(Features), [](const StringRef F) 
{
+return F.contains("crypto");
+  });
+  if (CryptoIt != Features.rend()) {
+if (CryptoIt->take_front() == "+") {
+  StringRef ArchSuffix = arm::getLLVMArchSuffixForARM(
+  arm::getARMTargetCPU(CPUName, ArchName, Triple), ArchName, Triple);
+  if (llvm::ARM::parseArchVersion(ArchSuffix) >= 8 &&
+  llvm::ARM::parseArchProfile(ArchSuffix) ==
+  llvm::ARM::ProfileKind::A) {
+if (ArchName.find_lower("+nosha2") == StringRef::npos &&
+CPUName.find_lower("+nosha2") == StringRef::npos)
+  Features.push_back("+sha2");
+if (ArchName.find_lower("+noaes") == StringRef::npos &&
+CPUName.find_lower("+noaes") == StringRef::npos)
+  Features.push_back("+aes");
+  } else {
+D.Diag(clang::diag::warn_target_unsupported_extension)
+<<

[PATCH] D67608: [ARM] Preserve fpu behaviour for '-crypto'

2019-09-16 Thread Peter Smith via Phabricator via cfe-commits

peter.smith added a comment.

After retesting on a failing example and experimenting a bit, I think that we 
should only preserve +crypto with -fno-integrated-as. It would also be good to 
mention D66018  and maybe add the original 
author as a reviewer.




Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:499
 << "crypto" << 
llvm::ARM::getArchName(llvm::ARM::parseArch(ArchSuffix));
-  Features.push_back("-crypto");
+  //-mfpu=crypto-neon-fp-armv8 does allow +sha2 / +aes / +crypto features,
+  // even if compiling for an cpu armv7a, if explicitly defined by the 
user,

Looking into this in more detail I think the comment can be made more specific. 
It seems like the main reason to keep +crypto is that when the 
non-integrated-assembler is used, then we get the directive:
```
.fpu crypto-neon-fp-armv8
```
In arm-none-eabi-as the assembler will support crypto instructions. Clang will 
not as any use of crypto instructions will fail due lack of v8 support.

```
With -fno-integrated-as -mfpu=crypto-neon-fp-armv8 some assemblers such as the 
GNU assembler will permit the use of crypto instructions as the fpu will 
override the architecture. We keep the crypto feature in this case to preserve 
compatibility. In all other cases we remove the crypto feature. 
```



Comment at: clang/lib/Driver/ToolChains/Arch/ARM.cpp:502
+  // so do not deactivate them.
+  if ((llvm::find(Features, "+fp-armv8") == Features.end()) ||
+  (llvm::find(Features, "+neon") == Features.end()))

I think we should only do this for -fno-integrated-as as the integrated 
assembler will fail if give a crypto instruction even with this change. With 
the integrated assembler we get:

```
error: instruction requires: armv8
aese.8 q8, q9
```



Comment at: clang/test/Driver/arm-features.c:84
+// Check +crypto does not affect -march=armv7a -mfpu=crypto-neon-fp-armv8, but 
it does warn that +crypto has no effect
+// RUN: %clang -target arm-arm-none-eabi -march=armv7a 
-mfpu=crypto-neon-fp-armv8 -### -c %s 2>&1 | FileCheck 
-check-prefixes=CHECK-WARNONLY,ALL %s
+// RUN: %clang -target arm-arm-none-eabi -march=armv7a+aes 
-mfpu=crypto-neon-fp-armv8 -### -c %s 2>&1 | FileCheck 
-check-prefixes=CHECK-WARNONLY,ALL,CHECK-HASAES %s

arm-arm-none-eabi is a vendor specific triple? Better to use arm-none-eabi 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67608



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

[PATCH] D67608: [ARM] Preserve fpu behaviour for '-crypto'

2019-09-16 Thread Diogo N. Sampaio via Phabricator via cfe-commits

dnsampaio created this revision.
dnsampaio added reviewers: peter.smith, labrinea.
Herald added subscribers: cfe-commits, dmgreen, kristof.beyls.
Herald added a project: clang.

This patch restores the behaviour that -fpu overwrites the
architecture obtained from -march or -mcpu flags, not enforcing to
disable 'crypto' if march=armv7 and mfpu=neon-fp-armv8.
However, it does warn that 'crypto' is ignored when passing
mfpu=crypto-neon-fp-armv8.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D67608

Files:
  clang/lib/Driver/ToolChains/Arch/ARM.cpp
  clang/test/Driver/arm-features.c


Index: clang/test/Driver/arm-features.c
===
--- clang/test/Driver/arm-features.c
+++ clang/test/Driver/arm-features.c
@@ -79,3 +79,18 @@
 // RUN: %clang -target arm-arm-none-eabi -mcpu=cortex-m23+crypto -### -c %s 
2>&1 | FileCheck -check-prefix=CHECK-NOCRYPTO5 %s
 // CHECK-NOCRYPTO5: warning: ignoring extension 'crypto' because the {{.*}} 
architecture does not support it
 // CHECK-NOCRYPTO5-NOT: "-target-feature" "+crypto"{{.*}} "-target-feature" 
"+sha2" "-target-feature" "+aes"
+//
+// Check +crypto does not affect -march=armv7a -mfpu=crypto-neon-fp-armv8, but 
it does warn that +crypto has no effect
+// RUN: %clang -target arm-arm-none-eabi -march=armv7a 
-mfpu=crypto-neon-fp-armv8 -### -c %s 2>&1 | FileCheck 
-check-prefixes=CHECK-WARNONLY,ALL %s
+// RUN: %clang -target arm-arm-none-eabi -march=armv7a+aes 
-mfpu=crypto-neon-fp-armv8 -### -c %s 2>&1 | FileCheck 
-check-prefixes=CHECK-WARNONLY,ALL,CHECK-HASAES %s
+// RUN: %clang -target arm-arm-none-eabi -march=armv7a+sha2 
-mfpu=crypto-neon-fp-armv8 -### -c %s 2>&1 | FileCheck 
-check-prefixes=CHECK-WARNONLY,ALL,CHECK-HASSHA %s
+// RUN: %clang -target arm-arm-none-eabi -march=armv7a+sha2+aes 
-mfpu=crypto-neon-fp-armv8 -### -c %s 2>&1 | FileCheck 
-check-prefixes=CHECK-WARNONLY,ALL,CHECK-HASSHA,CHECK-HASAES %s
+// RUN: %clang -target arm-arm-none-eabi -march=armv7a+aes -mfpu=neon-fp-armv8 
-### -c %s 2>&1 | FileCheck -check-prefixes=ALL,CHECK-HASAES %s
+// RUN: %clang -target arm-arm-none-eabi -march=armv7a+sha2 
-mfpu=neon-fp-armv8 -### -c %s 2>&1 | FileCheck 
-check-prefixes=ALL,CHECK-HASSHA %s
+// RUN: %clang -target arm-arm-none-eabi -march=armv7a+sha2+aes 
-mfpu=neon-fp-armv8 -### -c %s 2>&1 | FileCheck 
-check-prefixes=ALL,CHECK-HASSHA,CHECK-HASAES %s
+// CHECK-WARNONLY: warning: ignoring extension 'crypto' because the 'armv7-a' 
architecture does not support it
+// ALL: "-target-feature"
+// CHECK-WARNONLY-NOT: "-target-feature" "-crypto"
+// CHECK-HASSHA-SAME:  "-target-feature" "+sha2"
+// CHECK-HASAES-SAME:  "-target-feature" "+aes"
+//
Index: clang/lib/Driver/ToolChains/Arch/ARM.cpp
===
--- clang/lib/Driver/ToolChains/Arch/ARM.cpp
+++ clang/lib/Driver/ToolChains/Arch/ARM.cpp
@@ -496,7 +496,12 @@
 } else {
   D.Diag(clang::diag::warn_target_unsupported_extension)
 << "crypto" << 
llvm::ARM::getArchName(llvm::ARM::parseArch(ArchSuffix));
-  Features.push_back("-crypto");
+  //-mfpu=crypto-neon-fp-armv8 does allow +sha2 / +aes / +crypto features,
+  // even if compiling for an cpu armv7a, if explicitly defined by the 
user,
+  // so do not deactivate them.
+  if ((llvm::find(Features, "+fp-armv8") == Features.end()) ||
+  (llvm::find(Features, "+neon") == Features.end()))
+Features.push_back("-crypto");
 }
   }
 


Index: clang/test/Driver/arm-features.c
===
--- clang/test/Driver/arm-features.c
+++ clang/test/Driver/arm-features.c
@@ -79,3 +79,18 @@
 // RUN: %clang -target arm-arm-none-eabi -mcpu=cortex-m23+crypto -### -c %s 2>&1 | FileCheck -check-prefix=CHECK-NOCRYPTO5 %s
 // CHECK-NOCRYPTO5: warning: ignoring extension 'crypto' because the {{.*}} architecture does not support it
 // CHECK-NOCRYPTO5-NOT: "-target-feature" "+crypto"{{.*}} "-target-feature" "+sha2" "-target-feature" "+aes"
+//
+// Check +crypto does not affect -march=armv7a -mfpu=crypto-neon-fp-armv8, but it does warn that +crypto has no effect
+// RUN: %clang -target arm-arm-none-eabi -march=armv7a -mfpu=crypto-neon-fp-armv8 -### -c %s 2>&1 | FileCheck -check-prefixes=CHECK-WARNONLY,ALL %s
+// RUN: %clang -target arm-arm-none-eabi -march=armv7a+aes -mfpu=crypto-neon-fp-armv8 -### -c %s 2>&1 | FileCheck -check-prefixes=CHECK-WARNONLY,ALL,CHECK-HASAES %s
+// RUN: %clang -target arm-arm-none-eabi -march=armv7a+sha2 -mfpu=crypto-neon-fp-armv8 -### -c %s 2>&1 | FileCheck -check-prefixes=CHECK-WARNONLY,ALL,CHECK-HASSHA %s
+// RUN: %clang -target arm-arm-none-eabi -march=armv7a+sha2+aes -mfpu=crypto-neon-fp-armv8 -### -c %s 2>&1 | FileCheck -check-prefixes=CHECK-WARNONLY,ALL,CHECK-HASSHA,CHECK-HASAES %s
+// RUN: %clang -target arm-arm-none-eabi -march=armv7a+aes -mfpu=neon-fp-armv8 -### -c %s 2>&1 | FileCheck

[PATCH] D67608: [ARM] Preserve fpu behaviour for '-crypto'

[PATCH] D67608: [ARM] Preserve fpu behaviour for '-crypto'

[PATCH] D67608: [ARM] Preserve fpu behaviour for '-crypto'

[PATCH] D67608: [ARM] Preserve fpu behaviour for '-crypto'

[PATCH] D67608: [ARM] Preserve fpu behaviour for '-crypto'

5 matches

Site Navigation

Mail list logo

Footer information