Page MenuHomePhabricator

[MC] Provide an MCTargetOptions to implementors of MCAsmBackendCtorTy, NFC
ClosedPublic

Authored by joelkevinjones on Jan 14 2016, 9:15 PM.

Details

Summary

Some targets, notably AArch64 for ILP32, have different relocation encodings based upon the ABI. This is an enabling change, so a future patch can use the ABIName from MCTargetOptions to chose which relocations to use. Tested using check-llvm.

The corresponding change to clang is in: http://reviews.llvm.org/D16538

Diff Detail

Repository
rL LLVM

Event Timeline

joelkevinjones retitled this revision from to [MD] Provide an MCTargetOptions to implementors of MCAsmBackendCtorTy, NFC.
joelkevinjones updated this object.
joelkevinjones added a subscriber: llvm-commits.
dsanders accepted this revision.Jan 22 2016, 5:58 AM
dsanders edited edge metadata.

This LGTM but I'd appreciate it if someone else LGTM'd it too.

You'll need to make corresponding changes to the other projects (e.g. clang) to avoid buildbot failures.

tools/dsymutil/DwarfLinker.cpp
599 ↗(On Diff #44964)

Does it matter that we don't initialize the ABI name here? I don't think it does but I thought I ought to double check.

tools/llvm-dwp/llvm-dwp.cpp
385–386 ↗(On Diff #44964)

Does it matter that we don't initialize the ABI name here? I don't think it does but I thought I ought to double check.

This revision is now accepted and ready to land.Jan 22 2016, 5:58 AM

I've made the necessary change to clang and put it in:

http://reviews.llvm.org/D16538

tools/dsymutil/DwarfLinker.cpp
599 ↗(On Diff #44964)

It shouldn't matter. The MCTargetOptions default constructor implicitly uses the std::string default constructor by using "ABIName()" in its member initializer list.

tools/llvm-dwp/llvm-dwp.cpp
385–386 ↗(On Diff #44964)

It shouldn't matter. The MCTargetOptions default constructor implicitly uses the std::string default constructor by using "ABIName()" in its member initializer list.

joelkevinjones retitled this revision from [MD] Provide an MCTargetOptions to implementors of MCAsmBackendCtorTy, NFC to [MC] Provide an MCTargetOptions to implementors of MCAsmBackendCtorTy, NFC.Jan 25 2016, 9:12 AM
joelkevinjones edited edge metadata.
joelkevinjones updated this object.
echristo accepted this revision.Jan 27 2016, 2:10 PM
echristo edited edge metadata.

This is OK. Thanks for your patience.

-eric

rengolin edited edge metadata.Jun 27 2016, 6:51 AM

This hasn't gone in yet. Was this change abandoned?

Thanks for the follow up. I haven't gotten back to the work that motivated
the change. Would it be better to somehow retract the change and start
fresh when I pick up the work again? I don't follow LLVM development
closely enough to know the proper procedure.

Joel Jones

The general idea still works for me. If you update and everything still works then I say submit it.

-eric

joelkevinjones updated this revision to Diff 62777.EditedJul 5 2016, 12:19 PM
joelkevinjones edited edge metadata.

Rebased against ToT. Passes "make check".

I've tried doing a commit using "arc commit --revision D16213", but I seem to have forgotten my password. Further, I'm 100% certain my email was set to the one at Apple. Could someone do the commit and the associated clang change for me?

I'm trying to reconcile how this patch should interact with the set of proposed patches rooted at: https://reviews.llvm.org/D21465
The have similar goals, that is to support ABIs in a uniform fashion.

Daniel, could you comment?

Joel

I'm trying to reconcile how this patch should interact with the set of proposed patches rooted at: https://reviews.llvm.org/D21465
The have similar goals, that is to support ABIs in a uniform fashion.

Daniel, could you comment?

Joel

I think your patch is the preferable approach but I can't reach a working state with just the approach that you're using. As a result, our patches end up working together to reach a working solution which is close to the ideal end result but has some short-term compromises in order to have something that works. The problem I have is that there are some areas affected by the N32/N64 ABI's that are impossible to move to the MCTargetOptions approach (see the llvm-dev thread for details). I've asked questions about those areas to see if Eric has a way to resolve them but those questions haven't been answered so I don't know if he sees something I don't. These problems mean that I see having unambiguous ABI information in the triple (like X86 and ARM do today) as the only possible way to get N32/N64 working properly. ABI information in the triple is sufficient by itself to make N32/N64 work but following the discussion on llvm-dev I've added extra patches on the end so that all targets end up using both the triple and MCTargetOptions (like ARM does today).

To summarize, I think both our patches should go ahead and I'll adapt my series to fit the changes you make.

Daniel:

Thanks for your comments. I'll take your advice and push forward.

Joel

joelkevinjones updated this revision to Diff 65366.EditedJul 25 2016, 9:49 AM

Trying to get "arc commit" to not complain about "generated from '', but current working copy root is '/llvm/trunk'"

This revision was automatically updated to reflect the committed changes.