Page MenuHomePhabricator

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

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

Details

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.Mar 13 2020, 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.
251

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
15984

Combine these two to a single condition.

15993

Similarly to above, canonicalize to one direction.

16004

Flip and early exit.

16009

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.Mar 17 2020, 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.Mar 24 2020, 8:00 AM

Fixed bug with missing offset in object file.

anil9 added a subscriber: anil9.Mar 25 2020, 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.

255

Indentation.

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

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

nemanjai accepted this revision.Mar 30 2020, 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.

253

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

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

ABI check?

15987

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

15990

Please remove unnecessary calls to getNode().

15996

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

This revision is now accepted and ready to land.Mar 30 2020, 12:24 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptApr 21 2020, 9:10 AM