Page MenuHomePhabricator

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

Authored by ahatanak on Apr 29 2015, 9:08 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

ahatanak updated this revision to Diff 24675.Apr 29 2015, 9:08 PM
ahatanak retitled this revision from to [ARM] Use function attribute "arm-long-calls".
ahatanak updated this object.
ahatanak edited the test plan for this revision. (Show Details)
ahatanak added reviewers: echristo, dexonsmith.
ahatanak added a subscriber: Unknown Object (MLST).

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

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 accepted this revision.Apr 30 2015, 1:28 PM
rengolin added a reviewer: 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!

This revision is now accepted and ready to land.Apr 30 2015, 1:28 PM
echristo edited edge metadata.May 6 2015, 2:03 PM

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

-eric

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 updated this revision to Diff 28985.Jul 2 2015, 6:39 PM
ahatanak edited edge metadata.

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 accepted this revision.Jul 4 2015, 11:59 AM
echristo edited edge metadata.

Looks good. Thanks for the changes to the patch!

-eric

This revision was automatically updated to reflect the committed changes.