[ARM] Use function attribute "arm-long-calls"

[ahatanak]

Summary

This patch makes changes in ARMTargetLowering::LowerCall to check the caller function for the existence of function attribute "arm-long-calls" and use it to decide if it should emit a long call. Inliner conservatively rejects inlining if the caller has "arm-long-calls" but the callee doesn't, or vice-versa.

[rengolin]

Hi Akira,

I think this change makes sense, but let me understand the reason for it first...

Inlining interworking into non-interworking would require emission of BLX in a non-interworking function, which can't happen.

Inlining non-interworking into interworking would possibly convert all BLs into BLXs in the non-interworking part, which could be branching to non-interworking functions, thus not necessarily working as expected. There are cases here that would work, but you're trying yo be conservative.

The other two cases are trivial.

Is that your reasoning? If that's so, the patch looks good to me. Is there anything else?

cheers,
--renato

[ahatanak]

Hi Renato,

I realized today the title and summary I wrote were terribly uninformative, but this patch is part of the work to make LTO and function multi-versioning work correctly. In order to accomplish this, we have to save the command line option (-arm-long-calls) in the IR as a function attribute and fix the backend to check for the attribute so that it can decide on a per-function basis whether it should be emitting long calls.

As for the inlining part, my reasoning is that it's probably fine to compile a function that doesn't have attribute "arm-long-calls" as a function that does have the attribute, but not the other way around as it might turn a call that was in range into a call that is out of range. I used the word "conservative" as this patch rejects both cases even though it might not be necessary to reject the first case. I'm not sure how the inlining decision affects interworking (I'm assuming arm-thumb interworking?) though.

[rengolin]

I think we're in agreement as to the safety of the inlining, and I'm not particularly worried about needing to inline non-interworking into interworking and vice versa, since this should be an extremely rare case.

LGTM, Thanks!

[echristo]

I see you've kept the override here, any reason? :)

-eric

[ahatanak]

In D9364#167199, @echristo wrote:

I see you've kept the override here, any reason? :)

I didn't rewrite attribute "arm-long-calls" in the IR in ARMISelLowering.cpp because MachineFunction::Fn is a pointer to a constant. Also, I thought "arm-long-calls" should be handled in lib/Target/ARM since it is an ARM-specific option, and hence didn't rewrite it in llc.cpp as I did to allow overriding "target-cpu" and "target-features" (those are target-independent options).

Did I understand your question correctly?

[ahatanak]

The updated patch defines a new ARM subtarget feature for "long-calls" and replaces all the uses of EnableARMLongCalls with the new feature. This is simpler than my previous approach that uses a function attribute as there is no need to add new code to allow cl::opt options to override the attribute or disable inlining. I also made changes to ARM's fast-isel to read the subtarget feature and updated the test case to make sure "long-calls" is honored at -O0 too.

[echristo]

Looks good. Thanks for the changes to the patch!

-eric

lib/Analysis/IPA/InlineCost.cpp

 /// \brief Test that there are no attribute conflicts between Caller and Callee
 ///        that prevent inlining.
 static bool functionsHaveCompatibleAttributes(Function *Caller,
                                               Function *Callee) {
   return attributeMatches(Caller, Callee, "target-cpu") &&
          attributeMatches(Caller, Callee, "target-features") &&
          attributeMatches(Caller, Callee, Attribute::SanitizeAddress) &&
          attributeMatches(Caller, Callee, Attribute::SanitizeMemory) &&
-         attributeMatches(Caller, Callee, Attribute::SanitizeThread);
+         attributeMatches(Caller, Callee, Attribute::SanitizeThread) &&
+         attributeMatches(Caller, Callee, "arm-long-calls");
 }
 
 InlineCost InlineCostAnalysis::getInlineCost(CallSite CS, Function *Callee,
                                              int Threshold) {
   // Cannot inline indirect calls.
   if (!Callee)
     return llvm::InlineCost::getNever();
 

lib/Target/ARM/ARMISelLowering.cpp

ARMTargetLowering::LowerCall(TargetLowering::CallLoweringInfo &CLI,
   // If the callee is a GlobalAddress/ExternalSymbol node (quite common, every
   // direct call is) turn it into a TargetGlobalAddress/TargetExternalSymbol
   // node so that legalize doesn't hack it.
   bool isDirect = false;
   bool isARMFunc = false;
   bool isLocalARMFunc = false;
   ARMFunctionInfo *AFI = MF.getInfo<ARMFunctionInfo>();
 
-  if (EnableARMLongCalls) {
+  if (EnableARMLongCalls.getNumOccurrences()
+          ? EnableARMLongCalls
+          : MF.getFunction()->hasFnAttribute("arm-long-calls")) {
     assert((Subtarget->isTargetWindows() ||
             getTargetMachine().getRelocationModel() == Reloc::Static) &&
            "long-calls with non-static relocation model!");
     // Handle a global address or an external symbol. If it's not one of
     // those, the target's already in a register, so we don't need to do
     // anything extra.
     if (GlobalAddressSDNode *G = dyn_cast<GlobalAddressSDNode>(Callee)) {
       const GlobalValue *GV = G->getGlobal();

test/CodeGen/ARM/fn-attr-long-calls.ll

+; RUN: llc -march arm -mcpu cortex-a8 -relocation-model=static %s -o - | FileCheck -check-prefix=NO-OPTION %s
+; RUN: llc -march arm -mcpu cortex-a8 -relocation-model=static %s -o - -arm-long-calls | FileCheck -check-prefix=LONGCALL %s
+; RUN: llc -march arm -mcpu cortex-a8 -relocation-model=static %s -o - -arm-long-calls=false | FileCheck -check-prefix=NO-LONGCALL %s
+
+; NO-OPTION-LABEL: {{_?}}caller0
+; NO-OPTION: blx {{r[0-9]+}}
+
+; LONGCALL-LABEL: {{_?}}caller0
+; LONGCALL: blx {{r[0-9]+}}
+
+; NO-LONGCALL-LABEL: {{_?}}caller0
+; NO-LONGCALL: bl {{_?}}callee0
+
+define i32 @caller0() #0 {
+entry:
+  tail call void @callee0()
+  ret i32 0
+}
+
+; NO-OPTION-LABEL: {{_?}}caller1
+; NO-OPTION: bl {{_?}}callee0
+
+; LONGCALL-LABEL: {{_?}}caller1
+; LONGCALL: blx {{r[0-9]+}}
+
+; NO-LONGCALL-LABEL: {{_?}}caller1
+; NO-LONGCALL: bl {{_?}}callee0
+
+define i32 @caller1() {
+entry:
+  tail call void @callee0()
+  ret i32 0
+}
+
+declare void @callee0()
+
+attributes #0 = { "arm-long-calls" }

test/Transforms/Inline/attributes.ll

 
 define i32 @test_target_features1(i32 %i) "target-features"="+sse4.2" {
   %1 = call i32 @test_target_features_callee1(i32 %i)
   ret i32 %1
 ; CHECK-LABEL: @test_target_features1(
 ; CHECK-NEXT: @test_target_features_callee1
 ; CHECK-NEXT: ret i32
 }
+
+; Check that a function doesn't get inlined if "arm-long-calls" attribute
+; doesn't match.
+define i32 @test_arm_long_calls_callee0(i32 %i) {
+  ret i32 %i
+}
+
+define i32 @test_arm_long_calls_callee1(i32 %i) "arm-long-calls" {
+  ret i32 %i
+}
+
+define i32 @test_arm_long_calls0(i32 %i) {
+  %1 = call i32 @test_arm_long_calls_callee0(i32 %i)
+  %2 = call i32 @test_arm_long_calls_callee1(i32 %1)
+  ret i32 %1
+; CHECK-LABEL: @test_arm_long_calls0(
+; CHECK-NEXT: call i32 @test_arm_long_calls_callee1(
+; CHECK-NEXT: ret i32
+}
+
+define i32 @test_arm_long_calls1(i32 %i) "arm-long-calls" {
+  %1 = call i32 @test_arm_long_calls_callee0(i32 %i)
+  %2 = call i32 @test_arm_long_calls_callee1(i32 %1)
+  ret i32 %1
+; CHECK-LABEL: @test_arm_long_calls1(
+; CHECK-NEXT: call i32 @test_arm_long_calls_callee0(
+; CHECK-NEXT: ret i32
+}