Use function attribute "arm-restrict-it"

[ahatanak]

Summary

This patch changes ARMSubtarget::restrictIT to use the function attribute "arm-restrict-it" to decide whether ARMv7 or v8 style IT blocks should be generated.

IThe corresponding clang patch is here: http://reviews.llvm.org/D10414

[rengolin]

I'm confused. You removed the back-end option in Clang, but it remains here (IT), meaning you now have two identical command line options that do things slightly different. The function attributes set IT behaviour on every function and the backend option that sets it in a module level.

This will create a confusing environment on which to develop things and I don't want to extend this pattern. If the option needs to be on a function basis, than we need to make sure that whatever generates IR does that on a function level, and free the backend from having to ponder about module level options.

In summary, IT options must go. :)

cheers,
--renato

[echristo]

Hi Renato,

We've always had two options for this sort of thing, just much more
horribly implemented. :)

Basically the backend developers want to be able to override (largely for
debugging purposes) the "default" for whether or not to construct IT
blocks. It's just overriding the source level and it's good that we can do
this without having developers overwrite the source themselves (it could be
quite large).

I've delayed on reviewing this patch because I'm not quite sure how I want
particular target options to be passed to the backend, but in general the
idea of the patch ("allow the backend to override the IR") isn't a bad
thing.

-eric

[rengolin]

In D10416#191097, @echristo wrote:

We've always had two options for this sort of thing, just much more
horribly implemented. :)

I see. It is hidden, and it has the exact same behaviour (tri-state, isARMv8() on default, implemented in the same function), so I guess it's ok.

The chances that people will abuse of this relationship or will change one and not the other is indeed, minimal.

cheers,
--renato

[compnerd]

lib/Target/ARM/ARMSubtarget.cpp

320

Bikeshedding: I think that restrictedIT might be a better name. IIRC, the ARM ARM refers to this as Restricted IT Blocks.

test/CodeGen/Thumb2/restict-it-fnattr.ll

12

This test slightly concerns me. With Restricted IT blocks, IT blocks are only constructed under certain circumstances on ARMv7. Do you mind adding a bit more context around the test by adding -NEXT checks to ensure that the instructions within the IT blocks are exactly what we expect?

[ahatanak]

In D10416#191108, @rengolin wrote:

In D10416#191097, @echristo wrote:

We've always had two options for this sort of thing, just much more
horribly implemented. :)

I see. It is hidden, and it has the exact same behaviour (tri-state, isARMv8() on default, implemented in the same function), so I guess it's ok.

The chances that people will abuse of this relationship or will change one and not the other is indeed, minimal.

Since this is intended to be used only by tools such as llc and opt, we can move the IT option to CodeGen/CommandFlags.h and let setFunctionAttributes rewrite the function attribute in the IR. The downside of this is that we'll be moving a target-specific option into a file that is supposed to be target-independent.

[ahatanak]

Add checks in test case to ensure the instructions within the IT blocks are exactly what we expect.

[ahatanak]

lib/Target/ARM/ARMSubtarget.cpp

320

I'm not sure whether restrictIT is a good name or not, but if we are going to rename it, should we use a verb like "useRestrictedIT" as llvm's coding style guide recommends?

[ahatanak]

Following the discussion on cfe-commits (http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20150622/thread.html#131397), I've made changes to define a new ARM subtarget feature and use it to determine if IT blocks should be restricted. Previously, ARMSubtarget would check if the target supports armv8 instructions to make this decision. This is no longer needed as that decision is made in the front-end (see the corresponding clang patch: http://reviews.llvm.org/D10414).

[echristo]

I think you want to turn on restrict-it here:

def HasV8Ops : SubtargetFeature<"v8", "HasV8Ops", "true",

"Support ARM v8 instructions",
[HasV7Ops, FeatureVirtualization,
 FeatureMP]>;

by default yes?

-eric

[ahatanak]

Are you suggesting adding FeatureRestrictedIT to the implied feature list of HasV8Ops? I thought about doing so but found out it would make it impossible to disable restrict-it when we target v8. Try compiling the following IR with -mattr=-restrict-it after adding FeatureRestrictedIT to the implied feature list:

$ cat f1.ll
target datalayout = "e-m:e-p:32:32-i64:64-v128:64:128-a:0:32-n32-S64"
target triple = "armv8--linux-gnueabi"

define void @test_inlineasm() {
entry:

tail call void asm sideeffect "hlt #64", ""()
ret void

}

$ llc f1.ll -o - -mattr=-restrict-it
<inline asm>:1:2: error: instruction requires: armv8

hlt #64
^

LLVM ERROR: Error parsing inline asm

This fails because disabling restrict-it transitively disables HasV8Ops too (see function ClearImpliedBits in SubtargetFeature.cpp).

If I compile the IR with trunk's llc with command line option -arm-no-restrict-it, it compiles without any errors.

$ llc f1.ll -o - -arm-no-restrict-it

Do we need the capability to turn off disable restrict-it when targeting armv8? I was assuming we do need it although I wasn't sure why users would want to do that.

[echristo]

In D10416#198657, @ahatanak wrote:

Are you suggesting adding FeatureRestrictedIT to the implied feature list of HasV8Ops? I thought about doing so but found out it would make it impossible to disable restrict-it when we target v8. Try compiling the following IR with -mattr=-restrict-it after adding FeatureRestrictedIT to the implied feature list:

Fascinating.

This fails because disabling restrict-it transitively disables HasV8Ops too (see function ClearImpliedBits in SubtargetFeature.cpp).

Right.

If I compile the IR with trunk's llc with command line option -arm-no-restrict-it, it compiles without any errors.

$ llc f1.ll -o - -arm-no-restrict-it

Do we need the capability to turn off disable restrict-it when targeting armv8? I was assuming we do need it although I wasn't sure why users would want to do that.

I don't know. I wouldn't think so tbh which is why I just suggested putting it as a feature on the subarchitecture.

Jim?

-eric

[ahatanak]

r194592's commit log seems to suggest that both options were added to make llvm compatible with gcc.

http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20131111/194792.html
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20131111/194452.html

I guess we shouldn't disallow users to use "-mno-restrict-it" for ARMv8 even if it doesn't make much sense?

[echristo]

That was my thought. Let's have Jim weigh in because I like throwing him under this bus :)

-eric

[grosbach]

For the backend attribute, having -arm-restrict-it (or equivalent) transitively disable HasV8Ops is really, really bad. Those should be completely independent selections. The former is about tuning instruction selection (via if-conversion in this case) based on the microarchitecture. The latter is about which instructions are available in the first place. The former does not imply the latter.

I don't think there's much point to disabling restrict-it when targeting v8 from an end-user POV, but it's pretty useful for the backend. We want to be able to write test cases that make sure it's working, for example.

Do I understand right there's a frontend option, too? That's the only context in which gcc compatibility makes much sense. I don't see any value other than slavishly following gcc's option list to having this be exposed to end users in clang. I'd much rather it be entirely in the backend.

[ahatanak]

In D10416#241642, @grosbach wrote:

For the backend attribute, having -arm-restrict-it (or equivalent) transitively disable HasV8Ops is really, really bad. Those should be completely independent selections. The former is about tuning instruction selection (via if-conversion in this case) based on the microarchitecture. The latter is about which instructions are available in the first place. The former does not imply the latter.

I think we want to discuss whether the latter (HasV8Ops) implies the former (restrict-it). If it makes no sense from the user's POV to not restrict IT blocks when targeting armv8, then we can conclude armv8 implies restrict-it and add FeatureRestrictedIT to the implied features list of HasV8Ops. If we do this, disabling restrict-it would transitively disable HasV8Ops. Does that make sense?

I don't think there's much point to disabling restrict-it when targeting v8 from an end-user POV, but it's pretty useful for the backend. We want to be able to write test cases that make sure it's working, for example.

That said, if we want to allow llvm developers to turn off restrict-it when targeting v8 for debugging purposes, we shouldn't add FeatureRestrictedIT to the implied features list of HasV8Ops.

Do I understand right there's a frontend option, too? That's the only context in which gcc compatibility makes much sense. I don't see any value other than slavishly following gcc's option list to having this be exposed to end users in clang. I'd much rather it be entirely in the backend.

The front-end options are -mrestrict-it and -mno-restrict-it, which were committed in r194593. The clang-side patch that I submitted for review is here: http://reviews.llvm.org/D10414.

I agree that the option to disable restrict-it when targeting armv8 should be a backend option only available to llvm developers. If we all agree on that, I'll update the clang patch (D10414) accordingly.

No changes are needed to the llvm patch (this patch) to allow disabling restrict-it when targeting armv8. If -mattr=+restrict-it is not on the tool's (llc's) command line, restrict-it is disabled.

[grosbach]

In D10416#241764, @ahatanak wrote:

In D10416#241642, @grosbach wrote:

For the backend attribute, having -arm-restrict-it (or equivalent) transitively disable HasV8Ops is really, really bad. Those should be completely independent selections. The former is about tuning instruction selection (via if-conversion in this case) based on the microarchitecture. The latter is about which instructions are available in the first place. The former does not imply the latter.

I think we want to discuss whether the latter (HasV8Ops) implies the former (restrict-it). If it makes no sense from the user's POV to not restrict IT blocks when targeting armv8, then we can conclude armv8 implies restrict-it and add FeatureRestrictedIT to the implied features list of HasV8Ops. If we do this, disabling restrict-it would transitively disable HasV8Ops. Does that make sense?

It makes sense right up until you say, "If we do this, disabling restrict-it would transitively disable HasV8Ops." That may be the current implementation, but it's not what we want.

I don't think there's much point to disabling restrict-it when targeting v8 from an end-user POV, but it's pretty useful for the backend. We want to be able to write test cases that make sure it's working, for example.

That said, if we want to allow llvm developers to turn off restrict-it when targeting v8 for debugging purposes, we shouldn't add FeatureRestrictedIT to the implied features list of HasV8Ops.

Right. We want it to be the case by default for armv8, but not inextricably tied to it. It's a default, not a requirement.

Do I understand right there's a frontend option, too? That's the only context in which gcc compatibility makes much sense. I don't see any value other than slavishly following gcc's option list to having this be exposed to end users in clang. I'd much rather it be entirely in the backend.

The front-end options are -mrestrict-it and -mno-restrict-it, which were committed in r194593. The clang-side patch that I submitted for review is here: http://reviews.llvm.org/D10414.

I agree that the option to disable restrict-it when targeting armv8 should be a backend option only available to llvm developers. If we all agree on that, I'll update the clang patch (D10414) accordingly.

No changes are needed to the llvm patch (this patch) to allow disabling restrict-it when targeting armv8. If -mattr=+restrict-it is not on the tool's (llc's) command line, restrict-it is disabled.

[ahatanak]

Perhaps I should ask Weiming who committed the original patch to add these options just to be sure.

Is there a use case in which an end user wants to disable restrict-it when targeting armv8?

Would there be problems if we stopped exposing option -mno-restrict-it to the end user as suggested by Jim? I'm considering making changes to clang so that -mno-restrict-it has no effect on the backend code generation (end-users can use the option but it'll have no effect).

[weimingz]

Hi Akira,

Yes, the option "-mno-restrict-it " was added for GCC compatibility. I don't find a actual use case :)
So I think it should be OK to move to backend option only

In D10416#241967, @ahatanak wrote:

Perhaps I should ask Weiming who committed the original patch to add these options just to be sure.

Is there a use case in which an end user wants to disable restrict-it when targeting armv8?

Would there be problems if we stopped exposing option -mno-restrict-it to the end user as suggested by Jim? I'm considering making changes to clang so that -mno-restrict-it has no effect on the backend code generation (end-users can use the option but it'll have no effect).

[ahatanak]

In D10416#242134, @weimingz wrote:

Hi Akira,

Yes, the option "-mno-restrict-it " was added for GCC compatibility. I don't find a actual use case :)
So I think it should be OK to move to backend option only

OK, I'll make changes to clang to stop exposing it to the end-user as a front-end option.

[ahatanak]

Rebase to r247192 and remove a RUN line in test/CodeGen/Thumb2/restict-it-fnattr.ll that is no longer needed.

lib/Target/ARM/ARMBaseInstrInfo.cpp

bool ARMBaseInstrInfo::isPredicable(MachineInstr *MI) const {
 
   if (!isEligibleForITBlock(MI))
     return false;
 
   ARMFunctionInfo *AFI =
     MI->getParent()->getParent()->getInfo<ARMFunctionInfo>();
 
   if (AFI->isThumb2Function()) {
-    if (getSubtarget().restrictIT())
+    if (getSubtarget().restrictIT(*MI->getParent()->getParent()->getFunction()))
       return isV8EligibleForIT(MI);
   } else { // non-Thumb
     if ((MI->getDesc().TSFlags & ARMII::DomainMask) == ARMII::DomainNEON)
       return false;
   }
 
   return true;
 }

lib/Target/ARM/ARMSubtarget.h

protected:
   /// particularly effective at zeroing a VFP register.
   bool HasZeroCycleZeroing;
 
   /// AllowsUnalignedMem - If true, the subtarget allows unaligned memory
   /// accesses for some types.  For details, see
   /// ARMTargetLowering::allowsMisalignedMemoryAccesses().
   bool AllowsUnalignedMem;
 
-  /// RestrictIT - If true, the subtarget disallows generation of deprecated IT
-  ///  blocks to conform to ARMv8 rule.
-  bool RestrictIT;
-
   /// Thumb2DSP - If true, the subtarget supports the v7 DSP (saturating arith
   /// and such) instructions in Thumb2 code.
   bool Thumb2DSP;
 
   /// NaCl TRAP instruction is generated instead of the regular TRAP.
   bool UseNaClTrap;
 
   /// Target machine allowed unsafe FP math (such as use of NEON fp)
public:
   bool isR9Reserved() const { return IsR9Reserved; }
 
   bool useMovt(const MachineFunction &MF) const;
 
   bool supportsTailCall() const { return SupportsTailCall; }
 
   bool allowsUnalignedMem() const { return AllowsUnalignedMem; }
 
-  bool restrictIT() const { return RestrictIT; }
+  /// Return true if generating deprecated IT blocks is not allowed.
+  bool restrictIT(const Function &F) const;
 
   const std::string & getCPUString() const { return CPUString; }
 
   bool isLittle() const { return IsLittle; }
 
   unsigned getMispredictionPenalty() const;
 
   /// This function returns true if the target has sincos() routine in its

lib/Target/ARM/ARMSubtarget.cpp

 #include "ARMInstrInfo.h"
 #include "ARMMachineFunctionInfo.h"
 #include "ARMSelectionDAGInfo.h"
 #include "ARMSubtarget.h"
 #include "ARMTargetMachine.h"
 #include "Thumb1FrameLowering.h"
 #include "Thumb1InstrInfo.h"
 #include "Thumb2InstrInfo.h"
+#include "llvm/ADT/StringSwitch.h"
 #include "llvm/CodeGen/MachineRegisterInfo.h"
 #include "llvm/IR/Attributes.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/GlobalValue.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Target/TargetInstrInfo.h"
 #include "llvm/Target/TargetOptions.h"
 #include "llvm/Target/TargetRegisterInfo.h"
void ARMSubtarget::initSubtargetFeatures(StringRef CPU, StringRef FS) {
   } else {
     AllowsUnalignedMem = !(Align == StrictAlign);
   }
 
   // No v6M core supports unaligned memory access (v6M ARM ARM A3.2)
   if (isV6M())
     AllowsUnalignedMem = false;
 
-  switch (IT) {
-  case DefaultIT:
-    RestrictIT = hasV8Ops();
-    break;
-  case RestrictedIT:
-    RestrictIT = true;
-    break;
-  case NoRestrictedIT:
-    RestrictIT = false;
-    break;
-  }
-
   // NEON f32 ops are non-IEEE 754 compliant. Darwin is ok with it by default.
   const FeatureBitset &Bits = getFeatureBits();
   if ((Bits[ARM::ProcA5] || Bits[ARM::ProcA8]) && // Where this matters
       (Options.UnsafeFPMath || isTargetDarwin()))
     UseNEONForSinglePrecisionFP = true;
 }
 
 bool ARMSubtarget::isAPCS_ABI() const {
if (RelocM == Reloc::PIC_) {
       if (!GV->hasHiddenVisibility())  // Non-hidden $non_lazy_ptr reference.
         return true;
     }
   }
 
   return false;
 }
 
+bool ARMSubtarget::restrictIT(const Function &F) const {
+  // The default behavior is to disallow generation of deprecated IT blocks
+  // if the target architecture is armv8.
+  if (IT.getNumOccurrences() == 0) {
+    auto Val = F.getFnAttribute("arm-restrict-it").getValueAsString();
+    return StringSwitch<bool>(Val)
+        .Case("true", true)
+        .Case("false", false)
+        .Default(hasV8Ops());
+  }
+
+  switch (IT) {
+  case DefaultIT:
+    return hasV8Ops();
+  case RestrictedIT:
+    return true;
+  case NoRestrictedIT:
+    return false;
+  }
+}
+
 unsigned ARMSubtarget::getMispredictionPenalty() const {
   return SchedModel.MispredictPenalty;
 }
 
 bool ARMSubtarget::hasSinCos() const {
   return getTargetTriple().isiOS() && !getTargetTriple().isOSVersionLT(7, 0);
 }
 

lib/Target/ARM/ARMTargetMachine.cpp

void ARMPassConfig::addPreSched2() {
 
   // Expand some pseudo instructions into multiple instructions to allow
   // proper scheduling.
   addPass(createARMExpandPseudoPass());
 
   if (getOptLevel() != CodeGenOpt::None) {
     // in v8, IfConversion depends on Thumb instruction widths
     addPass(createThumb2SizeReductionPass([this](const Function &F) {
-      return this->TM->getSubtarget<ARMSubtarget>(F).restrictIT();
+      return this->TM->getSubtarget<ARMSubtarget>(F).restrictIT(F);
     }));
 
     addPass(createIfConverter([this](const Function &F) {
       return !this->TM->getSubtarget<ARMSubtarget>(F).isThumb1Only();
     }));
   }
   addPass(createThumb2ITBlockPass());
 }

lib/Target/ARM/Thumb2ITBlockPass.cpp

 bool Thumb2ITBlockPass::runOnMachineFunction(MachineFunction &Fn) {
   const ARMSubtarget &STI =
       static_cast<const ARMSubtarget &>(Fn.getSubtarget());
   if (!STI.isThumb2())
     return false;
   AFI = Fn.getInfo<ARMFunctionInfo>();
   TII = static_cast<const Thumb2InstrInfo *>(STI.getInstrInfo());
   TRI = STI.getRegisterInfo();
-  restrictIT = STI.restrictIT();
+  restrictIT = STI.restrictIT(*Fn.getFunction());
 
   if (!AFI->isThumbFunction())
     return false;
 
   bool Modified = false;
   for (MachineFunction::iterator MFI = Fn.begin(), E = Fn.end(); MFI != E; ) {
     MachineBasicBlock &MBB = *MFI;
     ++MFI;

test/CodeGen/Thumb2/restict-it-fnattr.ll

+; RUN: llc < %s -mtriple=thumbv7 -mcpu=cortex-a8 | FileCheck %s -check-prefix=NO-OPTION
+; RUN: llc < %s -mtriple=thumbv7 -mcpu=cortex-a8 -arm-no-restrict-it | FileCheck %s -check-prefix=NO-RESTRICT-IT
+; RUN: llc < %s -mtriple=thumbv7 -mcpu=cortex-a8 -arm-restrict-it | FileCheck %s -check-prefix=RESTRICT-IT
+
+; NO-OPTION-LABEL: {{\_?}}no_restrict_it:
+; NO-OPTION: itt
+
+; NO-RESTRICT-IT-LABEL: {{\_?}}no_restrict_it:
+; NO-RESTRICT-IT: itt
+
+; RESTRICT-IT-LABEL: {{\_?}}no_restrict_it:
+; RESTRICT-IT: it{{ }}
+
+define i32 @no_restrict_it(i32 %a, i32 %b) #0 {
+entry:
+  %cmp = icmp sgt i32 %a, 5
+  br i1 %cmp, label %if.then, label %if.end
+
+if.then:
+  %sub = add nsw i32 %a, -1
+  %add = add nsw i32 %b, 2
+  br label %if.end
+
+if.end:
+  %t0.0 = phi i32 [ %sub, %if.then ], [ %a, %entry ]
+  %t1.0 = phi i32 [ %add, %if.then ], [ %b, %entry ]
+  %mul = mul nsw i32 %t1.0, %t0.0
+  ret i32 %mul
+}
+
+; NO-OPTION-LABEL: {{\_?}}restrict_it:
+; NO-OPTION: it{{ }}
+
+; NO-RESTRICT-IT-LABEL: {{\_?}}restrict_it:
+; NO-RESTRICT-IT: itt
+
+; RESTRICT-IT-LABEL: {{\_?}}restrict_it:
+; RESTRICT-IT: it{{ }}
+
+define i32 @restrict_it(i32 %a, i32 %b) #1 {
+entry:
+  %cmp = icmp sgt i32 %a, 5
+  br i1 %cmp, label %if.then, label %if.end
+
+if.then:
+  %sub = add nsw i32 %a, -1
+  %add = add nsw i32 %b, 2
+  br label %if.end
+
+if.end:
+  %t0.0 = phi i32 [ %sub, %if.then ], [ %a, %entry ]
+  %t1.0 = phi i32 [ %add, %if.then ], [ %b, %entry ]
+  %mul = mul nsw i32 %t1.0, %t0.0
+  ret i32 %mul
+}
+
+attributes #0 = { "arm-restrict-it"="false" }
+attributes #1 = { "arm-restrict-it"="true" }