Page MenuHomePhabricator

[PowerPC][Future] Add offsets to PC Relative relocations.
AcceptedPublic

Authored by stefanp on Fri, Mar 13, 1:54 PM.

Details

Reviewers
nemanjai
lei
hfinkel
Group Reviewers
Restricted Project
Summary

This is an optimization that applies to global addresses and allows for the following transformation:
Convert this:

paddi r3, 0, symbol@PCREL, 1
ld r4, 8(r3)

To this:

pld r4, symbol@PCREL+8(0), 1

An instruction is saved and the linker can do the addition when the symbol is resolved.

Diff Detail

Event Timeline

stefanp created this revision.Fri, Mar 13, 1:54 PM

I think it would be useful to add a binary test case using either llvm-objdump or readelf if that is available. Just to test the desired encoding.

llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCCodeEmitter.cpp
199

Can we just simplify this by making it explicit with an early return?

if (!MO.isExpr())
  return getMachineOpValue(...)
// The rest of the code is for expressions.
249

Perhaps simplify this by canonicalizing to Reloc+Offset?

if (LHS->getKind != MCExpr::SymbolRef)
  std::swap(LHS, RHS);
if (LHS->getKind() != MCExpr::SymbolRef ||
    RHS->getKind() != MCExpr::Constant)
  llvm_unreachable("Expecting to have one constant and one relocation.");
const MCSymbolRefExpr *SRE = cast<MCSymbolRefExpr>(LHS);
const MCConstantExpr *CE = cast<MCConstantExpr>(RHS);
llvm/lib/Target/PowerPC/PPCISelLowering.cpp
15659

Combine these two to a single condition.

15668

Similarly to above, canonicalize to one direction.

15679

Flip and early exit.

15684

Are you only creating an APInt object to check that it fits? If so, you should just use isInt<34>(NewOffset) and avoid using APInt altogether.

stefanp updated this revision to Diff 250880.Tue, Mar 17, 1:13 PM

Addressed review comments for the code.
Added a test case that will check both the assembly and do an llvm-objdump of the object file.

stefanp updated this revision to Diff 252326.Tue, Mar 24, 8:00 AM

Fixed bug with missing offset in object file.

anil9 added a subscriber: anil9.Wed, Mar 25, 10:13 PM
anil9 added inline comments.
llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCCodeEmitter.cpp
198

s/compile time not/ compile time, not/

210

Generally the comments are above the code, here it seems the comments like Case1) are after the cases.

253

Indentation.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
15679

Does choosing to initialize DL depend on whether you swapped the operands earlier or not in line 15666 ?

nemanjai accepted this revision.Mon, Mar 30, 12:24 PM

Aside from a few minor nits, LGTM.

llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCCodeEmitter.cpp
210

It may be confusing to mention Case 1). Just describe what the case is.

251

Maybe an assert that the incoming value fits in 34 bits.

llvm/lib/Target/PowerPC/PPCISelLowering.cpp
15657

ABI check?

15662

In the other function, we use LHS/RHS. Please use that convention here as well for consistency.

15665

Please remove unnecessary calls to getNode().

15671

Just a comment above that operand zero of MAT_PCREL_ADDR is the GA node.

This revision is now accepted and ready to land.Mon, Mar 30, 12:24 PM