Page MenuHomePhabricator

[ARM] GlobalISel: Select globals in PIC mode
ClosedPublic

Authored by rovka on Aug 9 2017, 2:22 AM.

Details

Summary

Support the selection of G_GLOBAL_VALUE in the PIC relocation model. For
simplicity we use the same pseudoinstructions for both Darwin and ELF:
LDRLIT_ga_pcrel(_ldr). On Darwin we also use MOV_ga_pcrel(_ldr) if
available, but not on ELF (see PR28229).

This is new for ELF, so it requires a small update to the ARM pseudo
expansion pass to make sure it adds the correct constant pool modifier
and add-current-address in the case of ELF.

Diff Detail

Repository
rL LLVM

Event Timeline

rovka created this revision.Aug 9 2017, 2:22 AM

A potential drawback of using MOVW and MOVT in ELF is that the maximum immediate that can be used is a signed 16-bit value due to the constraint in ARMELF Section 4.6.1.1

For relocations processing MOVW and MOVT instructions (in both ARM and Thumb state), the initial addend is formed by interpreting the 16-bit literal field of the instruction as a 16-bit signed value in the range -32768 <= A < 32768. The interpretation is the same whether the relocated place contains a MOVW instruction or a MOVT instruction.

This is captured in https://bugs.llvm.org/show_bug.cgi?id=28229

I'm not familiar enough with ARMExpandPseudoInsts.cpp to know if it guarantees the addend won't overflow this range, or if you can select MOVT/MOVW only when you know the addend won't overflow? I suspect that for an overflow to occur it would require more than one global in the same section and each one accessed via a common base pointer + offset. For what its worth ARM's proprietary compiler used a separate RELA relocation section for MOVT/MOVW so that the addend could be encoded in the relocation; I don't think that this will work for llvm as not all linker's can cope with RELA relocations for ARM.

To the best of my knowledge the code and tests looks fine, but it is best if someone more familiar with the area gives it a check.

fhahn added a subscriber: fhahn.Aug 9 2017, 8:38 AM
rovka updated this revision to Diff 110551.Aug 10 2017, 4:23 AM
rovka edited the summary of this revision. (Show Details)

Thanks for the comments! I've updated the code to never use MOVT for ELF. I've also reworded the FIXME and mentioned the bug report in it.

Thanks for the update. I'm happy with the decision to not support MOVT/MOVW in ELF at this stage. I've no further comments.

rovka added a comment.Aug 21 2017, 9:47 AM

@t.p.northover Could you have a quick look at this, please?

t.p.northover accepted this revision.Aug 28 2017, 7:44 AM

Looks reasonable to me.

This revision is now accepted and ready to land.Aug 28 2017, 7:44 AM
This revision was automatically updated to reflect the committed changes.