Re: [PATCH] D10414: Attach function attribute "arm-restrict-it" instead of passing arm-restrict-it as a backend-option

[James Molloy via cfe-commits]

jmolloy added a subscriber: jmolloy.
jmolloy added a comment.

Hi Akira,

I'm sorry to be contrary (and I missed the discussion on Tuesday because I was 
away on vacation) but I think there *is* a usecase for -mno-restrict-it to 
work, and I would hate to see it broken.

Non-restricted IT blocks are indeed deprecated for ARMv8 in the ARMARM. But 
there are circumstances where you may still want to emit them - the biggest 
example being you're compiling for a CPU microarchitecture that you *know* 
doesn't have a performance penalty on non-restricted IT blocks. Restricted IT 
blocks can pessimize code quite badly in some circumstances, and allowing 
people to turn it off for their target if needed is very important, IMO.

Cheers,

James


http://reviews.llvm.org/D10414



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

Re: [PATCH] D10414: Attach function attribute "arm-restrict-it" instead of passing arm-restrict-it as a backend-option

[Jim Grosbach via cfe-commits]

grosbach added a comment.

In http://reviews.llvm.org/D10414#243056, @jmolloy wrote:

> Non-restricted IT blocks are indeed deprecated for ARMv8 in the ARMARM. But 
> there are circumstances where you may still want to emit them - the biggest 
> example being you're compiling for a CPU microarchitecture that you *know* 
> doesn't have a performance penalty on non-restricted IT blocks. Restricted IT 
> blocks can pessimize code quite badly in some circumstances, and allowing 
> people to turn it off for their target if needed is very important, IMO.


Bother, email response isn't showing up in Phab. Reposting here so there's a 
record. Sorry for the duplication on-list.

If such microarchitectures exist, shouldn’t they be represented properly as a 
CPU in the backend and get the right setting by default?


http://reviews.llvm.org/D10414



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

Re: [PATCH] D10414: Attach function attribute "arm-restrict-it" instead of passing arm-restrict-it as a backend-option

[James Molloy via cfe-commits]

jmolloy added a comment.

Hi Jim,

In an ideal world, yes. However there's no guarantee that all ARM implementors 
will (a) be able to commit to LLVM or (b) use ToT. Perhaps they're building a 
project that uses clang, or a specific version of clang, and this tuning option 
makes things go faster on their architecture.

I'm not arguing about the defaults, just about the breakage of -mno-restrict-it.

Cheers,

James


http://reviews.llvm.org/D10414



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

Re: [PATCH] D10414: Attach function attribute "arm-restrict-it" instead of passing arm-restrict-it as a backend-option

[Akira Hatanaka via cfe-commits]

ahatanak updated this revision to Diff 34379.
ahatanak added a comment.

Based on the feedback from reviewers for the llvm patch 
(http://reviews.llvm.org/D10416), I made changes to fix the handling of 
-mrestrict-it and -mno-restrict-it in the driver. The driver now passes 
subtarget feature "restrict-it"  if the target requires it (target is windows 
or armv8) or -mrestrict-it is on the command line and -mno-restrict-it doesn't 
appear after it. The end-user can no longer use -mno-restrict-it to instruct 
the backend to emit the deprecated (unrestricted) forms of IT blocks when 
targeting windows or armv8.


http://reviews.llvm.org/D10414

Files:
  lib/Driver/Tools.cpp
  test/Driver/arm-restrict-it.c
  test/Driver/woa-restrict-it.c

Index: test/Driver/woa-restrict-it.c
===
--- test/Driver/woa-restrict-it.c
+++ test/Driver/woa-restrict-it.c
@@ -1,4 +1,4 @@
-// RUN: %clang -target armv7-windows -### %s 2>&1 | FileCheck %s
-
-// CHECK: "-backend-option" "-arm-restrict-it"
+// RUN: %clang -target armv7-windows -### %s 2>&1 | FileCheck 
--check-prefix=CHECK-RESTRICTED %s
+// RUN: %clang -target armv7-windows -mno-restrict-it -### %s 2>&1 | FileCheck 
--check-prefix=CHECK-RESTRICTED %s
 
+// CHECK-RESTRICTED: "-target-feature" "+restrict-it"
Index: test/Driver/arm-restrict-it.c
===
--- test/Driver/arm-restrict-it.c
+++ test/Driver/arm-restrict-it.c
@@ -1,15 +1,21 @@
-// RUN: %clang -target arm-none-gnueabi -mrestrict-it -### %s 2> %t
+// RUN: %clang -target arm-none-gnueabi -mno-restrict-it -mrestrict-it -### %s 
2> %t
 // RUN: FileCheck --check-prefix=CHECK-RESTRICTED < %t %s
 
-// RUN: %clang -target armv8a-none-gnueabi -mrestrict-it -### %s 2> %t
+// RUN: %clang -target armv8-none-gnueabi -mno-restrict-it -### %s 2> %t
 // RUN: FileCheck --check-prefix=CHECK-RESTRICTED < %t %s
 
-// CHECK-RESTRICTED: "-backend-option" "-arm-restrict-it"
+// RUN: %clang -target armv8a-none-gnueabi -### %s 2> %t
+// RUN: FileCheck --check-prefix=CHECK-RESTRICTED < %t %s
+
+// RUN: %clang -target armv8a-none-gnueabi -mno-restrict-it -### %s 2> %t
+// RUN: FileCheck --check-prefix=CHECK-RESTRICTED < %t %s
+
+// CHECK-RESTRICTED: "-target-feature" "+restrict-it"
 
 // RUN: %clang -target arm-none-gnueabi -mno-restrict-it -### %s 2> %t
 // RUN: FileCheck --check-prefix=CHECK-NO-RESTRICTED < %t %s
 
-// RUN: %clang -target armv8a-none-gnueabi -mno-restrict-it -### %s 2> %t
+// RUN: %clang -target arm-none-gnueabi -### %s 2> %t
 // RUN: FileCheck --check-prefix=CHECK-NO-RESTRICTED < %t %s
 
-// CHECK-NO-RESTRICTED: "-backend-option" "-arm-no-restrict-it"
+// CHECK-NO-RESTRICTED-NOT: "-target-feature" "+restrict-it"
Index: lib/Driver/Tools.cpp
===
--- lib/Driver/Tools.cpp
+++ lib/Driver/Tools.cpp
@@ -795,10 +795,21 @@
   Features.push_back("-crc");
   }
 
-  if (Triple.getSubArch() == llvm::Triple::SubArchType::ARMSubArch_v8_1a) {
+  auto SubArch = Triple.getSubArch();
+
+  if (SubArch == llvm::Triple::SubArchType::ARMSubArch_v8_1a) {
 Features.insert(Features.begin(), "+v8.1a");
   }
 
+  // Windows on ARM and ARMv8 expect restricted IT blocks.
+  if (Triple.isOSWindows() ||
+  SubArch == llvm::Triple::SubArchType::ARMSubArch_v8 ||
+  SubArch == llvm::Triple::SubArchType::ARMSubArch_v8_1a)
+Features.push_back("+restrict-it");
+  else if (Args.hasFlag(options::OPT_mrestrict_it, 
options::OPT_mno_restrict_it,
+false))
+Features.push_back("+restrict-it");
+
   // Look for the last occurrence of -mlong-calls or -mno-long-calls. If
   // neither options are specified, see if we are compiling for kernel/kext and
   // decide whether to pass "+long-calls" based on the OS and its version.
@@ -4366,23 +4377,6 @@
 break;
   }
 
-  if (Arg *A = Args.getLastArg(options::OPT_mrestrict_it,
-   options::OPT_mno_restrict_it)) {
-if (A->getOption().matches(options::OPT_mrestrict_it)) {
-  CmdArgs.push_back("-backend-option");
-  CmdArgs.push_back("-arm-restrict-it");
-} else {
-  CmdArgs.push_back("-backend-option");
-  CmdArgs.push_back("-arm-no-restrict-it");
-}
-  } else if (Triple.isOSWindows() &&
- (Triple.getArch() == llvm::Triple::arm ||
-  Triple.getArch() == llvm::Triple::thumb)) {
-// Windows on ARM expects restricted IT blocks
-CmdArgs.push_back("-backend-option");
-CmdArgs.push_back("-arm-restrict-it");
-  }
-
   // Forward -f options with positive and negative forms; we translate
   // these by hand.
   if (Arg *A = Args.getLastArg(options::OPT_fprofile_sample_use_EQ)) {


Index: test/Driver/woa-restrict-it.c
===
--- test/Driver/woa-restrict-it.c
+++ test/Driver/woa-restrict-it.c
@@ -1,4 +1,4 @@
-// RUN: %clang -target armv7-windows -### %s