This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add lowering of addressing sequences for PIC
ClosedPublic

Authored by lewis-revill on Dec 4 2018, 5:08 PM.

Details

Summary

This patch allows lowering of PIC addresses by using PC-relative addressing for DSO-local symbols and accessing the address through the global offset table for non-DSO-local symbols.

Builds on D54143

Diff Detail

Repository
rL LLVM

Event Timeline

lewis-revill created this revision.Dec 4 2018, 5:08 PM
jrtc27 requested changes to this revision.Dec 4 2018, 7:03 PM

Half of this (the MC side of the GOT modifier) is already proposed in D55279. Also note that the assembly modifier is not %got_hi but %got_pcrel_hi after a discussion with upstream (check the assembler manual) to clearly convey that it's a PC-relative relocation to the GOT entry rather than generating a GOT index as would be the case on some older architectures. I did however, like you, use GOT_HI in the various LLVM-internal enumeration entries to match the name of the relocation.

This revision now requires changes to proceed.Dec 4 2018, 7:03 PM
lewis-revill planned changes to this revision.Dec 5 2018, 9:09 AM

That's an interesting coincidence! I think I'll rebase this patch on top of that patch since it seems likely to make it upstream.

It's probably worth noting that the %pcrel_lo relocations to the label appear to be evaluated to a constant when -mattr=+relax is not provided... I remember someone said that this was going to be made default? If so I think it would be good to wait for that before this is commited.

Update to use the MC layer changes in D55279. There are still some MC layer changes left in this patch waiting for D55279 to be updated.

jrtc27 added a comment.Dec 6 2018, 5:35 AM

It's probably worth noting that the %pcrel_lo relocations to the label appear to be evaluated to a constant when -mattr=+relax is not provided... I remember someone said that this was going to be made default? If so I think it would be good to wait for that before this is commited.

Ah, that would be because D54029 isn't yet merged.

Rebased with updated dependency.

lewis-revill planned changes to this revision.Dec 10 2018, 8:48 AM

Rebasing with latest changes to D55279 and modifying this patch to use and expand PseudoLA (D55325).

Rebased and updated to use and expand PseudoLA.

Almost there now from my point of view, but we'll see what the others say (especially Alex).

lib/Target/RISCV/RISCVISelLowering.cpp
353

Can we combine this with getAddr, pushing the common isPositionIndependent() check inside getAddr? Then the UseGOT flag becomes NonLocal or similar (or alternatively invert it and make it an IsLocal flag), and happens to only influence code generation for PIC. This should simplify all its uses.

jrtc27 added inline comments.Dec 12 2018, 8:36 AM
lib/Target/RISCV/RISCVISelLowering.h
116

Flags is never provided so it's always 0; can we drop it?

Remove unnecessary Flags operand

lewis-revill marked an inline comment as done.Dec 13 2018, 1:33 AM
lewis-revill added inline comments.
lib/Target/RISCV/RISCVISelLowering.cpp
353

I'm not sure this is a good idea? We'd still have to check in lowerGlobalAddress for GOT, and I feel leaving getAddr to check isPositionIndependent seems misleading, and the other option is to end up checking isPositionIndependent twice for this path, and gaining nothing in the process. It's certainly doable though, if people think it is clearer.

jrtc27 added inline comments.Dec 13 2018, 5:01 AM
lib/Target/RISCV/RISCVISelLowering.cpp
353

No you don’t, all you need to do is check shouldAssumeDSOLocal and pass that to getAddr as an IsLocal flag. The fact that the flag is only actually used on the PIC branch of getAddr is an implementation detail that lowerGlobalAddress doesn’t need to know about.

lewis-revill retitled this revision from [RISCV, WIP] Add lowering of addressing sequences for PIC to [RISCV] Add lowering of addressing sequences for PIC.

Move getAddrPIC() and isPositionIndependent() checks to getAddr()

lewis-revill marked an inline comment as done.Dec 13 2018, 9:50 AM
lewis-revill added inline comments.
lib/Target/RISCV/RISCVISelLowering.cpp
353

I tried this and it did look cleaner so I implemented it. Thanks for the suggestion.

Minor comment above but other than that this looks good to me too.

lib/Target/RISCV/RISCVExpandPseudoInsts.cpp
616 ↗(On Diff #178089)

Perhaps it is clearer if we call expandLoadLocalAddress here instead of repeating the call to expandAuipcInstPair.

Rebased and updated comments.

jrtc27 added a comment.Apr 3 2019, 7:30 AM

One minor comment, but otherwise I think this is ready to land now?

lib/Target/RISCV/RISCVExpandPseudoInsts.cpp
616 ↗(On Diff #178089)

I think it's better to match RISCVAsmParser::emitLoadAddress, which would make this something like:

unsigned SecondOpcode;
unsigned FlagsHi;
if (MF->getTarget().isPositionIndependent()) {
  const auto &STI = MF->getSubtarget<RISCVSubtarget>();
  SecondOpcode = STI.is64Bit() ? RISCV::LD : RISCV::LW;
  FlagsHi = RISCVII::MO_GOT_HI;
} else {
  SecondOpcode = RISCV::ADDI;
  FlagsHi = RISCVII::MO_PCREL_HI,
}
return expandAuipcInstPair(MBB, MBBI, NextMBBI, FlagsHi,
                           SecondOpcode);
Herald added a project: Restricted Project. · View Herald TranscriptApr 3 2019, 7:30 AM
Herald added subscribers: benna, psnobl. · View Herald Transcript

Rebased and modified expandLoadAddress.

mhorne added a subscriber: mhorne.Jun 1 2019, 10:56 AM
asb accepted this revision.Jun 11 2019, 3:00 AM

Please add RV64 RUN lines to pic-models.ll and add nounwind attributes, otherwise this looks good to me. Thanks!

This revision was not accepted when it landed; it landed in state Needs Review.Jun 11 2019, 5:54 AM
This revision was automatically updated to reflect the committed changes.