Attach function attribute "arm-restrict-it" instead of passing arm-restrict-it as a backend-option

[ahatanak]

Summary

This patch is part of the work to make LTO and function multi-versioning work correctly.

Currently, -mrestrict-it and -mno-restrict-it have no effect when doing an LTO build because cc1 passes them as backend options.

This patch fixes this bug by defining a new string function attribute "arm-restrict-it" and attaching it to the functions in the IR. This attribute is a tri-state, whose value corresponds to the cl:opt option "IT" n ARMSubtarget:

attribute not attached => DefaultIT ("arm-default-it")
arm-restrict-it=true => RestrictedIT ("arm-restrict-it")
arm-restrict-it=false => NoRestrictedIT ("arm-no-restrict-it")

I plan to send a corresponding llvm patch shortly.

[rengolin]

I'm not too familiar with the Clang option parsing and LTO to make a judgement here, but from an ARM's point of view, this looks ok.

You're not seem to be testing the third state. You need some CHECK-NOT tests to make sure they won't come when you don't want them.

[echristo]

Another possibility is that this gets mapped as a normal command line option passed into cc1 and then it can just be a subtarget feature?

Idle thoughts of different implementation, but it avoids a lot of complication and per-target free form attributes.

-eric

[ahatanak]

In D10414#191915, @echristo wrote:

Another possibility is that this gets mapped as a normal command line option passed into cc1 and then it can just be a subtarget feature?

Would it look something like this in the IR?

"target-features"="+neon,+vfp3,arn-restrict-it=false"

[ahatanak]

Based on suggestions from Eric, I've made changes to my previous patch to attach restrict-it to the target-features instead of attaching it as a function attribute. In order to avoid defining two new ARM subtarget features in the backend, I made changes to the driver to decide whether IT blocks should be restricted based on the triple and sub-arch. Subtarget feature +restrict-it is attached if the OS is Windows or the sub-arch is v8, unless the user overrides it with "-mno-restrict-it".

Relevant mailing list discussion:
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20150622/thread.html#131397

[ahatanak]

Update test cases.

[ahatanak]

I forgot to mention that I've removed the endianness check for arm or thumb on windows in my new patch. The old code only restricted IT for little endian targets, but as far as I know, big-endian is not supported for ARM on Windows, so I didn't think it was necessary to do the check there.

[rengolin]

In D10414#196900, @ahatanak wrote:

I forgot to mention that I've removed the endianness check for arm or thumb on windows in my new patch. The old code only restricted IT for little endian targets, but as far as I know, big-endian is not supported for ARM on Windows, so I didn't think it was necessary to do the check there.

An assert, then?

[ahatanak]

I found out there is a line in ARMTargetInfo::setABIAAPCS() that checks the endianness:

assert(!BigEndian && "Windows on ARM does not support big endian");

Not sure if this is the best place to do the check, but it should assert if the target is ARM big endian on windows.

[echristo]

I like this much more. Can we get a CodeGen check for this? LGTM otherwise.

Thanks!

-eric

[ahatanak]

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.

[jmolloy]

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

[grosbach]

In 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?

[jmolloy]

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

[grosbach]

In D10414#243302, @jmolloy wrote:

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.

Hmmm. I'm not sure I follow? In either case, why does what we do in clang matter at all? Especially for case (a); if they're not using LLVM, clang is completely irrelevant to them, right?

For (b), are you thinking of internal use by implementors when they're trying to decide what the right defaults should be for their tuning of the backend? In which case, would not an -mllvm option suffice?

The clang -m[no-]restrict-it is for end-users of clang, and that really doesn't seem the right thing to me. This is not a tuning parameter. It's a "what does the architecture I'm targeting support?" parameter. We only have even the backend option available for our own testing or we could (and should) get rid of that, too.

lib/Driver/Tools.cpp

static void getARMTargetFeatures(const Driver &D, const llvm::Triple &Triple,
   // En/disable crc code generation.
   if (Arg *A = Args.getLastArg(options::OPT_mcrc, options::OPT_mnocrc)) {
     if (A->getOption().matches(options::OPT_mcrc))
       Features.push_back("+crc");
     else
       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.
   if (Arg *A = Args.getLastArg(options::OPT_mlong_calls,
                                options::OPT_mno_long_calls)) {
     if (A->getOption().matches(options::OPT_mlong_calls))
       Features.push_back("+long-calls");
   } else if (KernelOrKext && (!Triple.isiOS() || Triple.isOSVersionLT(6))) {

   case llvm::Triple::thumbeb:
     CmdArgs.push_back("-fallow-half-arguments-and-returns");
     break;
 
   default:
     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)) {
     StringRef fname = A->getValue();
     if (!llvm::sys::fs::exists(fname))
       D.Diag(diag::err_drv_no_such_file) << fname;
     else
       A->render(Args, CmdArgs);

test/Driver/arm-restrict-it.c

-// 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"

test/Driver/woa-restrict-it.c

-// 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"