Page MenuHomePhabricator

[ARM] Save option "arm-long-calls" to the IR as a function attribute
ClosedPublic

Authored by ahatanak on Apr 30 2015, 1:00 PM.

Details

Summary

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

Currently, -mlong-calls, which is converted to cc1 option -arm-long-calls, is ignored when building with LTO because the option isn't passed to the linker or libLTO. This patch saves the option in the IR as a function attribute to fix this problem.

The corresponding llvm patch is here:
http://reviews.llvm.org/D9364

There are a few things to discuss:

  1. Should "arm-long-calls" be a binary attribute or a tri-state like "unsafe-fp-math" that takes a value "true" or "false"? I made it a binary attribute because it simplifies the backend and frontend without breaking backward compatibility, but might be use cases that I'm unaware of in which this approach wouldn't work.
  1. Since we are saving the option in the IR, should we stop passing it as a cc1 backend option and stop passing it to llvm:๐Ÿ†‘:ParseCommandLineOptions? It's not needed if this attribute is a tri-state, but is needed if it's a binary to preserve backward compatibility. By "backward compatibility", I mean the following commands should produce the same result before and after this patch is committed:
  1. clang -target armv7-apple-ios -static -mlong-calls old.bc -o old
  2. clang -target armv7-apple-ios -static old.bc -o old

Here, old.bc is generated by an older version of clang that doesn't save "arm-long-calls" in the IR.

Diff Detail

Repository
rL LLVM

Event Timeline

ahatanak updated this revision to Diff 24762.Apr 30 2015, 1:00 PM
ahatanak retitled this revision from to [ARM] Save option "arm-long-calls" to the IR as a function attribute.
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).
ahatanak updated this revision to Diff 25383.May 8 2015, 4:18 PM

Made changes based on feedback comments.

In the new patch, a new cc1 option "function-attribute" is defined and "arm-long-calls" is passed as a function-attribute instead of a backend-option. I think this is better than my previous patch as it doesn't have to check "arm-long-calls", which is a target-specific attribute, in CompilerInvocation.cpp, and it no longer passes the string to cl::ParseCommandLineOptions to toggle the cl::opt option in the backend.

echristo edited edge metadata.Jul 1 2015, 11:01 PM

Hi Akira,

I'm not sure how we want to do calling convention type things. I don't know that we want it as a generic attribute on the function or as a subtarget feature. What are your thoughts here?

-eric

It seems to me that turning arm-long-calls into a subtarget feature would simplify both clang and llvm.

ahatanak updated this revision to Diff 28984.Jul 2 2015, 6:19 PM
ahatanak edited edge metadata.

Save "long-calls" as a subtarget feature of ARM instead of a function attribute.

echristo accepted this revision.Jul 4 2015, 12:04 PM
echristo edited edge metadata.

Couple of inline comments, go ahead and commit when you've resolved them.

Thanks!

-eric

lib/Driver/Tools.cpp
715โ€“716 โ†—(On Diff #28984)

Can you add a plain english comment above this explaining what we're supposed to be checking here? :)

test/CodeGen/arm-long-calls.c
3 โ†—(On Diff #28984)

Test for -mno-long-calls?

This revision is now accepted and ready to land.Jul 4 2015, 12:04 PM
ahatanak updated this revision to Diff 29150.Jul 6 2015, 11:05 PM
ahatanak edited edge metadata.

Add comments to explain how the decision to pass feature "+long-calls" is made.

ahatanak marked an inline comment as done.Jul 6 2015, 11:12 PM
ahatanak added inline comments.
test/CodeGen/arm-long-calls.c
4 โ†—(On Diff #29150)

"+long-calls" (or "-long-calls") is never passed if -mno-calls is specified on the command line. The driver test (arm-long-calls.c) checks that "+long-calls" gets passed based on occurrence of "-mlong-calls" or "-mno-long-calls".

This revision was automatically updated to reflect the committed changes.
ahatanak marked an inline comment as done.