Page MenuHomePhabricator

[PowerPC] Extend .reloc directive on PowerPC
ClosedPublic

Authored by stefanp on May 8 2020, 3:47 AM.

Details

Summary

A linker optimization is available on PowerPC for GOT indirect PCRelative loads.

When the compiler generates a GOT indirect load it must generate two loads. One
that loads the address of the element from the GOT and a second to load the
actual element based on the address just loaded from the GOT. However, the
linker can optimize these two loads into one load if it knows that it is safe
to do so. The compiler can tell the linker that the optimization is safe
by using the R_PPC64_PCREL_OPT relocation. The relocation can be used as follows

  pld 3, vec@got@pcrel(0), 1
.Lpcrel1:
    ... More instructions possible here ...
.reloc .Lpcrel1-8,R_PPC64_PCREL_OPT,.-(.Lpcrel1-8)
  lwa 3, 4(3)

The first load to get the address from the GOT has a label immediately following
it (in this case Lpcrel1) and then the second load has the .reloc directive that
actually attaches the relocation to the first load using the Lpcrel1-8. The end
result is that the first load from the GOT has a relocation on it that specifies
the offset in the text section to the second load so that the linker can perform
the optimization.
There is another possible form for this setup as follows:

  pld 3, vec@got@pcrel(0), 1
.Lpcrel1=.-8
      ... More instructions possible here ...
.reloc .Lpcrel1,R_PPC64_PCREL_OPT,.-.Lpcrel1
  lwa 3, 4(3)

In the second form the label is variable and contains the .-8.
This patch expands the .reloc directive to support the second form from above. The first form was added in D83751.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Address Review Comments

nemanjai added inline comments.Jun 19 2020, 2:58 PM
llvm/lib/MC/MCExpr.cpp
325 ↗(On Diff #262850)

It seems to me that <<Invalid>> may be the right thing to do to clearly signal that this thing isn't supposed to be emitted. Is there something else (other than getVariantKindForName) that needs this to have some value such as the one you chose?

442 ↗(On Diff #271440)

Since this isn't supposed to be printed and the choice of the string is somewhat arbitrary, do we even want to add it here? It seems that VK_Invalid may actually be the right thing to do here.

llvm/lib/Target/PowerPC/AsmParser/PPCAsmParser.cpp
1813 ↗(On Diff #271440)

I think an example of what this actually looks like here would be useful. It may be obvious in this review as it is in the test case below, but it won't be obvious looking at the code in the future.

llvm/lib/Target/PowerPC/MCTargetDesc/PPCELFStreamer.cpp
153 ↗(On Diff #271440)

Is this really the condition under which we want to defer to the base class? In the parsing function, we handle a situation where the Offset expression is a binary expression and there is no relocation expression. So is it now possible to end up deferring to the base class with the equivalent of .reloc .Label1-8,SOME_RELOCATION_AGAINST_LAB_MINUS8?

I guess what I am getting at is that we should be checking for
Offset.getKind() == MCExpr::Binary

An assert is fine as well along the lines of:

assert((Expr || Offset.getKind() != MCExpr::Binary) &&
       "Expecting a relocation value for .reloc directives with a binary expression");
156 ↗(On Diff #271440)

// Get the MCFixupKind that corresponds to Name.

165 ↗(On Diff #271440)

Please replace uses of parent and parent class with base class.

176 ↗(On Diff #271440)

I think Anil's question makes sense. Is it possible for getOffsetFromBinaryExpr() to return true and not set DF?

llvm/test/MC/PowerPC/future-reloc-with-expr.s
2 ↗(On Diff #271440)
  1. Please add at least some testing where the instructions in between include prefixed instr alignment nops. Presumably you can do this by aligning the function to 64 bytes and then have 8 prefixed instructions and one non-prefixed (or use .space).
  2. Please add a test case where the pld itself is being realigned
  3. Please add a test case where the realigned pld has a label (both on the same line and on the line above)
107 ↗(On Diff #271440)

What is this nop? Is it to align the paddi below? If so, it seems to me that it is in the wrong place.

199 ↗(On Diff #271440)

I don't see the difference between this and the corresponding "non-variable symbol" test case. Can you point out to me what the difference is? Similarly with other variable symbol test cases below.

sfertile added inline comments.Jun 23 2020, 8:28 AM
llvm/include/llvm/MC/MCExpr.h
305 ↗(On Diff #272187)

I think the name should include PCREL since this Expr is for representing the PCREL_OPT relocation.

The other comment show you how to produce the variant kind. What about // .reloc expr, R_PPC64_PCREL_OPT, expr for the comment?

llvm/lib/MC/MCExpr.cpp
325 ↗(On Diff #262850)

Is the reason the string needs to be unique because its used in a sting switch in getVariantKindForName? If so mapping it to VK_Invalid in both here and there like Nemanja suggests seems like the right thing to do.

llvm/lib/Target/PowerPC/AsmParser/PPCAsmParser.cpp
1811 ↗(On Diff #272187)

Was the problem that the existing functionality can only parse a constant or symbol reference for the offset? If so why extend the functionality in the PPCASMParser as opposed to AsmParser::parseDirectiveReloc? I'm not super familiar with the .reloc directive but gas documents accepting symbol + offset for the first expression. https://sourceware.org/binutils/docs/as/Reloc.html#Reloc

stefanp marked 8 inline comments as done.Jun 23 2020, 12:16 PM
stefanp added inline comments.
llvm/include/llvm/MC/MCExpr.h
305 ↗(On Diff #272187)

I agree that the name with PCREL will work better.
I'm going to use VK_PPC_PCREL_OPT.

llvm/lib/MC/MCExpr.cpp
325 ↗(On Diff #262850)

No, there is nowhere else that I can think of that would require this name.
I agree that <<Invalid>> is a better choice.

442 ↗(On Diff #271440)

Agreed. I'm going to remove this completely. It is not required and if that string ever gets passed it will drop through to VK_Invalid anyway.

llvm/lib/Target/PowerPC/AsmParser/PPCAsmParser.cpp
1811 ↗(On Diff #272187)

This is a very good point: this is just an extension of existing functionality. I wanted to make it PPC specific because I believed that the community would more easily accept this code as PPC specific. I have nothing against making this target independent.
I would also like to hear form some of the other reviewers on this as to whether or not they believe that making this target independent would make more sense.

llvm/lib/Target/PowerPC/MCTargetDesc/PPCELFStreamer.cpp
153 ↗(On Diff #271440)

So, I ended up doing something different.
It seems that the base class assumes:

if (Expr == nullptr)
    Expr = MCSymbolRefExpr::create(getContext().createTempSymbol(),
                                   getContext());

The use the current position to fill in the symbol. So, I will do the same thing.
I will access the base class only as a fall-through.

176 ↗(On Diff #271440)

Ok, I see what was meant by that comment. Sorry, I misunderstood at first.
The issue is that DF is set in another function and even though it is correct that right now it is impossible to return true without setting DF I am adding the assert as a way to make sure that possible future changes to getOffsetFromBinaryExpr keep that.

llvm/test/MC/PowerPC/future-reloc-with-expr.s
107 ↗(On Diff #271440)

That nop is an alignment nop and is not intentional from the testing perspective but it does appear to be in the correct place.

0000000000000070 <PrefixInsnBetween>:
      70: 00 00 10 04 00 00 60 e4      	pld 3, 0(0), 1
		0000000000000070:  R_PPC64_GOT_PCREL34	vec
		0000000000000070:  R_PPC64_PCREL_OPT	*ABS*+0x28
      78: 2a 00 63 38  	addi 3, 3, 42
      7c: 00 00 00 60  	nop
      80: 00 00 00 06 2a 00 63 38      	paddi 3, 3, 42, 0
      88: 2a 00 63 38  	addi 3, 3, 42
      8c: 00 00 00 06 2a 00 63 38      	paddi 3, 3, 42, 0
      94: 2a 00 63 38  	addi 3, 3, 42
      98: 06 00 63 e8  	lwa 3, 4(3)
      9c: 20 00 80 4e  	blr

We cannot start a prefixed instruction at 0x7c so we add the nop.
We only use .p2align4 and so functions are only aligned to 16 bytes.

Having said all that, I can make it intentional! As you mention above a test with prefixed instructions and a nop is useful.

199 ↗(On Diff #271440)

That's because I forgot to change these ones to look like the test just above. I'll fix that.

stefanp updated this revision to Diff 272792.Jun 23 2020, 12:18 PM

Fixed a number of issues pointed out by the reviewers.

stefanp marked an inline comment as not done.Jun 29 2020, 9:16 AM
stefanp added inline comments.
llvm/lib/Target/PowerPC/AsmParser/PPCAsmParser.cpp
1811 ↗(On Diff #272187)

After further consideration, I think we should probably leave things separate for now. No other targets have needed these features yet and it would probably be better to implement them on PowerPC first and test them that way. After some of the bugs have been worked out we can propose to make a wider change that will work on all targets with ELF and have a second patch for that.

sfertile added inline comments.Jul 2 2020, 9:02 AM
llvm/lib/MC/MCExpr.cpp
325 ↗(On Diff #272792)

nit: Should be on one line.

llvm/lib/Target/PowerPC/AsmParser/PPCAsmParser.cpp
1814 ↗(On Diff #272792)

Do we want the example to show up in the doxygen?

1819 ↗(On Diff #272792)

A couple nits in regards to this comment:

The .reloc directive is consumed by the assembler to emit a relocation. It is clearer to separate the explanation of the directive, and then explain the relocations significance to the linker.

We use 'position' to refer to the expression which defines the relocations r_offset field, then use 'offset' to refer to the expression which defines the relocations r_addend field, which I find confusing.

Taking both those into account and stealing from the ABI description of the relocation:

///   pld 3, vec@got@pcrel(0), 1
/// .Lpcrel1:
///   .reloc .Lpcrel1-8,R_PPC64_PCREL_OPT,.-(.Lpcrel1-8)
///   lwa 3, 4(3)

/// The .reloc directive instructs the assembler to emit a relocation of type R_PPC64_RCREL_OPT, referencing 
/// offset `.Lpcrel1-8` (the pc-relative load from the got) with addend `.-(.Lpcrel1-8)` (the offset from 
/// got access instruction to the associated load/store instruction).
/// The relocation specifies that the instructions at r_offset (pld) and r_offset + r_addend (lwa) 
/// may be optimized by the linker (ie the compiler guarantees that register lifetimes are such 
/// that the optimization is safe).
1849 ↗(On Diff #272792)

Don't we need further checking on the BinaryExpr to make sure its a symbol+offset expression? I'm guessing we are relying on getOffsetFromBinaryExpr to catch the error right now, but a diagnostic should be emitted while parsing since we have OffsetLoc.

llvm/lib/Target/PowerPC/MCTargetDesc/PPCAsmBackend.cpp
72 ↗(On Diff #272792)

Shouldn't this be zero? The relocation is a hint to the linker that the 2 instructions it references are optimizable and doesn't represent any relocatable bits.

105 ↗(On Diff #272792)

Shouldn't this fixup be the same as fixup_ppc_nofixups?

118 ↗(On Diff #272792)

ditto.

llvm/lib/Target/PowerPC/MCTargetDesc/PPCELFStreamer.cpp
114 ↗(On Diff #272792)

Drop At this point.

118 ↗(On Diff #272792)

Comment is redundant, its obvious from the code that LHS is a SymbolRefExpr and RHS is a ConstantExpr.

120 ↗(On Diff #272792)

Why a static_cast instead of a 'cast'?

143 ↗(On Diff #272792)

Again why the static cast?

llvm/lib/Target/PowerPC/MCTargetDesc/PPCFixupKinds.h
46 ↗(On Diff #272792)
/// Not a true fixup, but ties a pc-relative got access to an associated  memory operation
/// to indicate to the linker that the sequence is safe to optimize.
stefanp updated this revision to Diff 276264.Jul 7 2020, 4:48 PM
stefanp marked 4 inline comments as done and an inline comment as not done.
stefanp edited the summary of this revision. (Show Details)

Added a couple of checks when reading the .reloc directive.
Fixed a number of comments.
Fixed the specifications for fixup_ppc_linker_opt.

stefanp added inline comments.Jul 7 2020, 4:49 PM
llvm/lib/Target/PowerPC/AsmParser/PPCAsmParser.cpp
1814 ↗(On Diff #272792)

Yes, we can add it to Doxygen.
I'm not sure how exactly it would look in this case (or if it would be formatted correctly) but it would be nice to have the full text. I assume that the \\\ will add anything after it to Doxygen.

1819 ↗(On Diff #272792)

I agree your description is much clearer.

1849 ↗(On Diff #272792)

Yes, we are relying on getOffsetFromBinaryExpr to catch the error.
However, I'll add another check in here.

llvm/lib/Target/PowerPC/MCTargetDesc/PPCAsmBackend.cpp
72 ↗(On Diff #272792)

Yes, you are right that this should be like fixup_ppc_nofixups. Initially I had considered this to be potentially "changing" all of the bits of the instruction it is optimizing but that doesn't really make a ton of sense. I'll change it.

nemanjai accepted this revision.Jul 10 2020, 6:36 AM

My remaining comments are minor and can be addressed on the commit. AFAICT, Sean's comments have been addressed as well so I think this is good to go. @sfertile if you have any objections, feel free to override my approval, otherwise this is good to go.

llvm/lib/Target/PowerPC/MCTargetDesc/PPCELFStreamer.cpp
111 ↗(On Diff #276264)

Please favour reference-to-pointer rather than pointer-to-pointer as I believe the former is the convention.

183 ↗(On Diff #276264)

I think you'll need (void)HaveOffset; to silence warnings on no asserts builds.

212 ↗(On Diff #276264)

Is this comment actually true? It seems that if the expression is not a binary expression, we assert.

223 ↗(On Diff #276264)

Similar to above wrt. unused variable without asserts.

This revision is now accepted and ready to land.Jul 10 2020, 6:36 AM
nemanjai added inline comments.Jul 10 2020, 11:44 AM
llvm/lib/Target/PowerPC/MCTargetDesc/PPCAsmBackend.cpp
118 ↗(On Diff #276264)

This is going to need MCFixupKindInfo::FKF_IsPCRel or else it needs to be moved to the non-PC-Rel section of the switch in getRelocType().

stefanp updated this revision to Diff 277512.Jul 13 2020, 11:42 AM
stefanp marked 2 inline comments as done.

Fixed warnings for no asserts builds.
Fixed text of comment that was out of date.
Moved fixup_ppc_linker_opt to the non-PCRel section of getRelocType.

stefanp added inline comments.Jul 13 2020, 11:55 AM
llvm/lib/Target/PowerPC/MCTargetDesc/PPCAsmBackend.cpp
118 ↗(On Diff #276264)

I think I'm going to end up moving this to the non PC-Rel section.

llvm/lib/Target/PowerPC/MCTargetDesc/PPCELFStreamer.cpp
212 ↗(On Diff #276264)

You are right. That comment is out of date. We do assert now.

stefanp updated this revision to Diff 277520.Jul 13 2020, 11:57 AM

Fixed a couple of spacing nits.

MaskRay added inline comments.
llvm/test/MC/PowerPC/pcrel-reloc-with-expr.s
1 ↗(On Diff #277520)

How about ppc64-reloc-directive-pcrel.s

There is a ppc64-reloc-directive.s

MaskRay requested changes to this revision.Jul 13 2020, 12:04 PM
This revision now requires changes to proceed.Jul 13 2020, 12:04 PM
MaskRay added inline comments.Jul 13 2020, 12:10 PM
llvm/include/llvm/MC/MCExpr.h
305 ↗(On Diff #277520)

I am not sure VK_PPC_PCREL_OPT is needed.

ELFPPCAsmBackend::getFixupKind will return FirstLiteralRelocationKind + R_PPC64_PCREL_OPT

llvm/lib/Target/PowerPC/AsmParser/PPCAsmParser.cpp
1665 ↗(On Diff #277520)

We should just reuse the generic .reloc parsing code and enhance it with .reloc . - 4, ... instead of adding a ppc64 specific clone.

MaskRay added a comment.EditedJul 13 2020, 12:11 PM

I added .reloc support to many targets (x86,arm,aarch64,risc-v,powerpc{32,64}). Requested changes as I think we should improve the generic parsing code, instead of adding a ppc64 specific clone. And this patch seems to be unnecessarily more complex than it can be.

MaskRay added a comment.EditedJul 13 2020, 11:34 PM

I added .reloc support to many targets (x86,arm,aarch64,risc-v,powerpc{32,64}). Requested changes as I think we should improve the generic parsing code, instead of adding a ppc64 specific clone. And this patch seems to be unnecessarily more complex than it can be.

I'll try fixing the generic parseDirectiveReloc tomorrow. parseDirectiveReloc and emitRelocDirective actually require some interface cleanup.

edit: D83751

stefanp updated this revision to Diff 278270.Jul 15 2020, 12:01 PM
stefanp edited the summary of this revision. (Show Details)

Moved the rest of the work to the target indep side after the patch D83751.

The patch still adds support for R_PPC64_PCREL_OPT and adds support for variable labels:

  pld 3, vec@got@pcrel(0), 1
.Lpcrel1=.-8       // Variable labels.
      ... More instructions possible here ...
.reloc .Lpcrel1,R_PPC64_PCREL_OPT,.-.Lpcrel1
  lwa 3, 4(3)
stefanp updated this revision to Diff 278556.Jul 16 2020, 11:30 AM

Forgot to rename the testcase in the previous update of the patch.
Renamed it now.

MaskRay added inline comments.Jul 17 2020, 1:27 PM
llvm/lib/MC/MCObjectStreamer.cpp
671

The verification part does not have a close relation with the rest of PPC specific handling. I think we should move this part to a separate patch.

IIUC, without the verification part, the functionality still exists. It is just that we can miss some erroneous cases.

llvm/lib/Target/PowerPC/MCTargetDesc/PPCELFObjectWriter.cpp
390 ↗(On Diff #278556)

Can we just use FirstLiteralRelocationKind+R_PPC64_PCREL_OPT and not define fixup_ppc_linker_opt?

Also, can you run clang-format so all the notifications go away and make this easier to review?

stefanp updated this revision to Diff 279315.Jul 20 2020, 11:33 AM
stefanp marked 2 inline comments as done.

Removed the fixup name.
@MaskRay Let me know if this is what you were looking for.

llvm/lib/MC/MCObjectStreamer.cpp
671

I'm not sure what you mean in this case.

I do need to have a separate case for the Symbol.isVariable() situation. Otherwise I get a "Cannot get offset for a common/variable symbol" error.
Do you mean I should remove checking like this:

if(!SymbolExpr->evaluateAsRelocatable(OffsetVal, nullptr, nullptr))
return std::make_pair(false,
    std::string("symbol in .reloc offset is not "
          "relocatable"));
llvm/lib/Target/PowerPC/MCTargetDesc/PPCELFObjectWriter.cpp
390 ↗(On Diff #278556)

Ok, I'll get rid of that the fixup name.
I'll update the patch and you can see if that is what you are looking for.

I do have a question related to this though... In the future I will want to generate fixups of this type. Usually I would do that like this:

MCFixup::create(Offset, Expr, (MCFixupKind)PPC::fixup_ppc_linker_opt, Loc);

If I completely remove this naming would I be doing this instead?

MCFixup::create(Offset, Expr, FirstLiteralRelocationKind, Loc);

Is that the right way to do it?

stefanp marked 2 inline comments as not done.Jul 20 2020, 11:34 AM
stefanp added inline comments.Jul 20 2020, 12:11 PM
llvm/lib/Target/PowerPC/MCTargetDesc/PPCELFObjectWriter.cpp
390 ↗(On Diff #278556)

Sorry!
I think I just figured out what you meant...
Let me update the patch again.

stefanp updated this revision to Diff 279325.EditedJul 20 2020, 12:21 PM

Updated patch to correctly remove new fixup.
In order to create the fixup I can use:

MCFixup::create(Offset, Expr, FirstLiteralRelocationKind + R_PPC64_PCREL_OPT, Loc);
MaskRay accepted this revision.Jul 20 2020, 4:15 PM

Looks great!

llvm/test/MC/PowerPC/ppc64-reloc-directive-pcrel.s
2

-unknown-unknown can be removed. powerpc64le works

This revision is now accepted and ready to land.Jul 20 2020, 4:15 PM
This revision was automatically updated to reflect the committed changes.