[PowerPC] Extend .reloc directive on PowerPC

[stefanp]

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.

[amyk]

llvm/lib/MC/MCExpr.cpp

325 ↗

(On Diff #262850)

I noticed for other PPC VariantKinds, we don't put the ppc part in the string. Is there a reason why we call it ppclinkeropt instead of just linkeropt? Is it since linkeropt might sound too generic?

llvm/lib/Target/PowerPC/MCTargetDesc/PPCELFStreamer.cpp

174 ↗

(On Diff #262850)

Maybe early exit/break here instead then return false at the end, like how it is in the MCExpr::Binary?

llvm/test/MC/PowerPC/future-reloc-with-expr.s

6 ↗

(On Diff #262850)

s/paremter/parameter

[NeHuang]

llvm/lib/Target/PowerPC/MCTargetDesc/PPCAsmBackend.cpp

50 ↗

(On Diff #262850)

nit: Can we move this to line 36 as we already have a case return Value.

166 ↗

(On Diff #262850)

Why do we need to add case FK_NONE?

[stefanp]

Address review comments.

llvm/lib/MC/MCExpr.cpp

325 ↗

(On Diff #262850)

This variant kind is a little bit different from the others. Other variant kinds use the string when generating assembly output files. For example when we use VK_PPC_GOT_PCREL we actually output symbol@got@pcrel into the assembly file. This string has to match what gcc does when it emits the symbol and relocation in the output. In the case of VK_PPC_LINKER_OPT we won't be doing that. The string "ppclinkeropt" should never get printed to the assembly file so it doesn't really matter what we name it as long as it is consistent in LLVM. I used the "ppc" prefix for it because the strings needs to be unique (see getVariantKindForName) and as a result I figured it would be less likely to conflict with a future string.

[anil9]

llvm/lib/Target/PowerPC/AsmParser/PPCAsmParser.cpp

1829 ↗

(On Diff #265996)

nit : change number to constant ?

1854 ↗

(On Diff #265996)

nit > 80 char

llvm/lib/Target/PowerPC/MCTargetDesc/PPCELFStreamer.cpp

126 ↗

(On Diff #265996)

Indentation.

148 ↗

(On Diff #265996)

nit > 80 char.

155 ↗

(On Diff #265996)

Indentation.

162 ↗

(On Diff #265996)

will this assertion be reached given the break just before it ?

llvm/lib/Target/PowerPC/MCTargetDesc/PPCFixupKinds.h

46 ↗

(On Diff #265996)

nit , fullstop.

[stefanp]

llvm/lib/Target/PowerPC/MCTargetDesc/PPCELFStreamer.cpp

126 ↗

(On Diff #265996)

It seems that clang-format thinks this is fine.
I checked what we did in other places in llvm/lib/Target/PowerPC/ and we seem to use both formats. Unless we can find a really compelling reason to change this I'm going to leave it as-is.

155 ↗

(On Diff #265996)

Again, I think this is fine since clang-format thinks it is fine.

162 ↗

(On Diff #265996)

The break is only triggered if the if statement is true. If !getOffsetFromBinaryExpr(BinExpr, ComputedOffset, &DF) is false we don't break.

[stefanp]

Address review comments.

[stefanp]

Added more comments and broke up confusing if statements.

[lei]

llvm/lib/Target/PowerPC/MCTargetDesc/PPCELFStreamer.cpp

147 ↗

(On Diff #271440)

nit: .relc -> .reloc

llvm/test/MC/PowerPC/future-reloc-with-expr.s

2 ↗

(On Diff #271440)

Should we use --mcpu=power10 since that's available?
Maybe the name of this file should also be updated? pcRel-reloc-with-expr.s?

2 ↗

(On Diff #271440)

do we need to add tests for BE?

[stefanp]

Address Review Comments

[nemanjai]

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]

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]

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]

Fixed a number of issues pointed out by the reviewers.

[stefanp]

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]

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]

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

[stefanp]

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]

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.

[nemanjai]

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]

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]

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]

Fixed a couple of spacing nits.

[MaskRay]

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]

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]

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]

In D79625#2148175, @MaskRay wrote:

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]

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]

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

[MaskRay]

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?

[nemanjai]

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

[stefanp]

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]

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 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]

Looks great!

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

2

-unknown-unknown can be removed. powerpc64le works

llvm/include/llvm/BinaryFormat/ELFRelocs/PowerPC64.def

 #undef R_PPC64_TLSLD
 #undef R_PPC64_ADDR16_HIGH
 #undef R_PPC64_ADDR16_HIGHA
 #undef R_PPC64_TPREL16_HIGH
 #undef R_PPC64_TPREL16_HIGHA
 #undef R_PPC64_DTPREL16_HIGH
 #undef R_PPC64_DTPREL16_HIGHA
 #undef R_PPC64_REL24_NOTOC
+#undef R_PPC64_PCREL_OPT
 #undef R_PPC64_PCREL34
 #undef R_PPC64_GOT_PCREL34
 #undef R_PPC64_IRELATIVE
 #undef R_PPC64_REL16
 #undef R_PPC64_REL16_LO
 #undef R_PPC64_REL16_HI
 #undef R_PPC64_REL16_HA
 

 ELF_RELOC(R_PPC64_TLSLD,                108)
 ELF_RELOC(R_PPC64_ADDR16_HIGH,          110)
 ELF_RELOC(R_PPC64_ADDR16_HIGHA,         111)
 ELF_RELOC(R_PPC64_TPREL16_HIGH,         112)
 ELF_RELOC(R_PPC64_TPREL16_HIGHA,        113)
 ELF_RELOC(R_PPC64_DTPREL16_HIGH,        114)
 ELF_RELOC(R_PPC64_DTPREL16_HIGHA,       115)
 ELF_RELOC(R_PPC64_REL24_NOTOC,          116)
+ELF_RELOC(R_PPC64_PCREL_OPT,            123)
 ELF_RELOC(R_PPC64_PCREL34,              132)
 ELF_RELOC(R_PPC64_GOT_PCREL34,          133)
 ELF_RELOC(R_PPC64_IRELATIVE,            248)
 ELF_RELOC(R_PPC64_REL16,                249)
 ELF_RELOC(R_PPC64_REL16_LO,             250)
 ELF_RELOC(R_PPC64_REL16_HI,             251)
 ELF_RELOC(R_PPC64_REL16_HA,             252)

llvm/lib/MC/MCObjectStreamer.cpp

void MCObjectStreamer::emitGPRel64Value(const MCExpr *Value) {
   MCDataFragment *DF = getOrCreateDataFragment();
   flushPendingLabels(DF, DF->getContents().size());
 
   DF->getFixups().push_back(
       MCFixup::create(DF->getContents().size(), Value, FK_GPRel_4));
   DF->getContents().resize(DF->getContents().size() + 8, 0);
 }
 
+static Optional<std::pair<bool, std::string>>
+getOffsetAndDataFragment(const MCSymbol &Symbol, uint32_t &RelocOffset,
+                         MCDataFragment *&DF) {
+  if (Symbol.isVariable()) {
+    const MCExpr *SymbolExpr = Symbol.getVariableValue();
+    MCValue OffsetVal;
+    if(!SymbolExpr->evaluateAsRelocatable(OffsetVal, nullptr, nullptr))
+      return std::make_pair(false,
+                            std::string("symbol in .reloc offset is not "
+                                        "relocatable"));
+    if (OffsetVal.isAbsolute()) {
+      RelocOffset = OffsetVal.getConstant();
+      MCFragment *Fragment = Symbol.getFragment();
+      // FIXME Support symbols with no DF. For example:
+      // .reloc .data, ENUM_VALUE, <some expr>
+      if (!Fragment || Fragment->getKind() != MCFragment::FT_Data)
+        return std::make_pair(false,
+                              std::string("symbol in offset has no data "
+                                          "fragment"));
+      DF = cast<MCDataFragment>(Fragment);
+      return None;
+    }
+
+    if (OffsetVal.getSymB())
+      return std::make_pair(false,
+                            std::string(".reloc symbol offset is not "
+                                        "representable"));
+
+    const MCSymbolRefExpr &SRE = cast<MCSymbolRefExpr>(*OffsetVal.getSymA());
+    if (!SRE.getSymbol().isDefined())
+      return std::make_pair(false,
+                            std::string("symbol used in the .reloc offset is "
+                                        "not defined"));
+
+    if (SRE.getSymbol().isVariable())
+      return std::make_pair(false,
+                            std::string("symbol used in the .reloc offset is "
+                                        "variable"));
+
+    MCFragment *Fragment = SRE.getSymbol().getFragment();
+    // FIXME Support symbols with no DF. For example:
+    // .reloc .data, ENUM_VALUE, <some expr>
+    if (!Fragment || Fragment->getKind() != MCFragment::FT_Data)
+      return std::make_pair(false,
+                            std::string("symbol in offset has no data "
+                                        "fragment"));
+    RelocOffset = SRE.getSymbol().getOffset() + OffsetVal.getConstant();
+    DF = cast<MCDataFragment>(Fragment);
+  } else {
+    RelocOffset = Symbol.getOffset();
+    MCFragment *Fragment = Symbol.getFragment();
+    // FIXME Support symbols with no DF. For example:
+    // .reloc .data, ENUM_VALUE, <some expr>
+    if (!Fragment || Fragment->getKind() != MCFragment::FT_Data)
+      return std::make_pair(false,
+                            std::string("symbol in offset has no data "
+                                        "fragment"));
+    DF = cast<MCDataFragment>(Fragment);
+  }
+  return None;
+}
+
 Optional<std::pair<bool, std::string>>
 MCObjectStreamer::emitRelocDirective(const MCExpr &Offset, StringRef Name,
                                      const MCExpr *Expr, SMLoc Loc,
                                      const MCSubtargetInfo &STI) {
   Optional<MCFixupKind> MaybeKind = Assembler->getBackend().getFixupKind(Name);
   if (!MaybeKind.hasValue())
     return std::make_pair(true, std::string("unknown relocation name"));
 
DF->getFixups().push_back(
         MCFixup::create(OffsetVal.getConstant(), Expr, Kind, Loc));
     return None;
   }
   if (OffsetVal.getSymB())
     return std::make_pair(false,
                           std::string(".reloc offset is not representable"));
 
   const MCSymbolRefExpr &SRE = cast<MCSymbolRefExpr>(*OffsetVal.getSymA());
-  if (SRE.getSymbol().isDefined()) {
-    // FIXME SRE.getSymbol() may not be relative to DF.
+  const MCSymbol &Symbol = SRE.getSymbol();
+  if (Symbol.isDefined()) {
+    uint32_t SymbolOffset = 0;
+    Optional<std::pair<bool, std::string>> Error;
+    Error = getOffsetAndDataFragment(Symbol, SymbolOffset, DF);
+
+    if (Error != None)
+      return Error;
+
     DF->getFixups().push_back(
-        MCFixup::create(SRE.getSymbol().getOffset() + OffsetVal.getConstant(),
+        MCFixup::create(SymbolOffset + OffsetVal.getConstant(),
                         Expr, Kind, Loc));
     return None;
   }
 
   PendingFixups.emplace_back(&SRE.getSymbol(), DF,
                              MCFixup::create(-1, Expr, Kind, Loc));
   return None;
 }

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

+# RUN: llvm-mc -triple=powerpc64le -filetype=obj %s | \
+# RUN: llvm-objdump -dr --mcpu=pwr10 - | FileCheck %s
+# RUN: llvm-mc -triple=powerpc64 -filetype=obj %s | \
+# RUN: llvm-objdump -dr --mcpu=pwr10 - | FileCheck %s
+
+
+##
+# This section of tests contains the MCBinaryExpr as the first parameter of the
+# .reloc relocation.
+##
+  .text
+  .abiversion 2
+  .globl  Minimal
+  .p2align  4
+  .type Minimal,@function
+Minimal:
+.LMinimal$local:
+  pld 3, vec@got@pcrel(0), 1
+.Lpcrel1:
+  .reloc .Lpcrel1-8,R_PPC64_PCREL_OPT,.-(.Lpcrel1-8)
+  lwa 3, 4(3)
+  blr
+  .long 0
+  .quad 0
+# CHECK-LABEL:   Minimal
+# CHECK:         pld 3, 0(0), 1
+# CHECK-NEXT:    R_PPC64_GOT_PCREL34  vec
+# CHECK-NEXT:    R_PPC64_PCREL_OPT  *ABS*+0x8
+# CHECK-NEXT:    lwa 3, 4(3)
+# CHECK-NEXT:    blr
+
+  .globl  SingleInsnBetween
+  .p2align  4
+  .type SingleInsnBetween,@function
+SingleInsnBetween:
+.LSingleInsnBetween$local:
+  pld 3, vec@got@pcrel(0), 1
+.Lpcrel2:
+        addi 3, 3, 42
+  .reloc .Lpcrel2-8,R_PPC64_PCREL_OPT,.-(.Lpcrel2-8)
+  lwa 3, 4(3)
+  blr
+  .long 0
+  .quad 0
+# CHECK_LABEL:   SingleInsnBetween
+# CHECK:         pld 3, 0(0), 1
+# CHECK-NEXT:    R_PPC64_GOT_PCREL34  vec
+# CHECK-NEXT:    R_PPC64_PCREL_OPT  *ABS*+0xc
+# CHECK-NEXT:    addi 3, 3, 42
+# CHECK-NEXT:    lwa 3, 4(3)
+# CHECK-NEXT:    blr
+
+
+  .globl  MultiInsnBetween                    # -- Begin function
+  .p2align  4
+  .type MultiInsnBetween,@function
+MultiInsnBetween:
+.LMultiInsnBetween$local:
+  pld 3, vec@got@pcrel(0), 1
+.Lpcrel3:
+        addi 3, 3, 42
+        addi 3, 3, 42
+        addi 3, 3, 42
+        addi 3, 3, 42
+        addi 3, 3, 42
+  .reloc .Lpcrel3-8,R_PPC64_PCREL_OPT,.-(.Lpcrel3-8)
+  lwa 3, 4(3)
+  blr
+  .long 0
+  .quad 0
+# CHECK_LABEL:   MultiInsnBetween
+# CHECK:         pld 3, 0(0), 1
+# CHECK-NEXT:    R_PPC64_GOT_PCREL34  vec
+# CHECK-NEXT:    R_PPC64_PCREL_OPT  *ABS*+0x1c
+# CHECK-NEXT:    addi 3, 3, 42
+# CHECK-NEXT:    addi 3, 3, 42
+# CHECK-NEXT:    addi 3, 3, 42
+# CHECK-NEXT:    addi 3, 3, 42
+# CHECK-NEXT:    addi 3, 3, 42
+# CHECK-NEXT:    lwa 3, 4(3)
+# CHECK-NEXT:    blr
+
+  .globl  PrefixInsnBetween
+  .p2align  6
+  .type PrefixInsnBetween,@function
+        .space          48       # Add a space to force an alignment of a paddi.
+PrefixInsnBetween:
+.LPrefixInsnBetween$local:
+  pld 3, vec@got@pcrel(0), 1
+.Lpcrel4:
+        addi 3, 3, 42
+        paddi 3, 3, 42, 0
+        addi 3, 3, 42
+        paddi 3, 3, 42, 0
+        addi 3, 3, 42
+  .reloc .Lpcrel4-8,R_PPC64_PCREL_OPT,.-(.Lpcrel4-8)
+  lwa 3, 4(3)
+  blr
+  .long 0
+  .quad 0
+# CHECK_LABEL:   PrefixInsnBetween
+# CHECK:         pld 3, 0(0), 1
+# CHECK-NEXT:    R_PPC64_GOT_PCREL34  vec
+# CHECK-NEXT:    R_PPC64_PCREL_OPT  *ABS*+0x28
+# CHECK-NEXT:    addi 3, 3, 42
+# CHECK-NEXT:    nop
+# CHECK-NEXT:    paddi 3, 3, 42, 0
+# CHECK-NEXT:    addi 3, 3, 42
+# CHECK-NEXT:    paddi 3, 3, 42, 0
+# CHECK-NEXT:    addi 3, 3, 42
+# CHECK-NEXT:    lwa 3, 4(3)
+# CHECK-NEXT:    blr
+
+
+  .globl  SpaceBetween                    # -- Begin function
+  .p2align  4
+  .type SpaceBetween,@function
+SpaceBetween:
+.LSpaceBetween$local:
+  pld 3, vec@got@pcrel(0), 1
+.Lpcrel5:
+        addi 3, 3, 42
+        paddi 3, 3, 42, 0
+        addi 3, 3, 42
+        .space 40, 0
+        paddi 3, 3, 42, 0
+        addi 3, 3, 42
+  .reloc .Lpcrel5-8,R_PPC64_PCREL_OPT,.-(.Lpcrel5-8)
+  lwa 3, 4(3)
+  blr
+  .long 0
+  .quad 0
+# CHECK_LABEL:   SpaceBetween
+# CHECK:         pld 3, 0(0), 1
+# CHECK-NEXT:    R_PPC64_GOT_PCREL34  vec
+# CHECK-NEXT:    R_PPC64_PCREL_OPT  *ABS*+0x50
+# CHECK-NEXT:    addi 3, 3, 42
+# CHECK-NEXT:    nop
+# CHECK-NEXT:    paddi 3, 3, 42, 0
+# CHECK-NEXT:    addi 3, 3, 42
+# CHECK:         paddi 3, 3, 42, 0
+# CHECK-NEXT:    addi 3, 3, 42
+# CHECK-NEXT:    lwa 3, 4(3)
+# CHECK-NEXT:    blr
+
+
+  .globl  Plus
+  .p2align  4
+  .type Plus,@function
+Plus:
+.LPlus$local:
+.Lpcrel6:
+        addi 3, 3, 42
+        addi 3, 3, 42
+  pld 3, vec@got@pcrel(0), 1
+  .reloc .Lpcrel6+8,R_PPC64_PCREL_OPT,.-(.Lpcrel6+8)
+  lwa 3, 4(3)
+  blr
+  .long 0
+  .quad 0
+# CHECK-LABEL:   Plus
+# CHECK:         pld 3, 0(0), 1
+# CHECK-NEXT:    R_PPC64_PCREL_OPT  *ABS*+0x8
+# CHECK-NEXT:    R_PPC64_GOT_PCREL34  vec
+# CHECK-NEXT:    lwa 3, 4(3)
+# CHECK-NEXT:    blr
+
+##
+# This section of tests contains the variable MCSymbol as part of the
+# MCSymbolRefExpr for the first parameter of the .reloc relocation.
+##
+  .globl  VarLabelMinimal                    # -- Begin function
+  .p2align  4
+  .type VarLabelMinimal,@function
+VarLabelMinimal:
+.LVarLabelMinimal$local:
+  pld 3, vec@got@pcrel(0), 1
+.Lpcrel101=.-8
+  .reloc .Lpcrel101,R_PPC64_PCREL_OPT,.-.Lpcrel101
+  lwa 3, 4(3)
+  blr
+  .long 0
+  .quad 0
+# CHECK-LABEL:   VarLabelMinimal
+# CHECK:         pld 3, 0(0), 1
+# CHECK-NEXT:    R_PPC64_GOT_PCREL34  vec
+# CHECK-NEXT:    R_PPC64_PCREL_OPT  *ABS*+0x8
+# CHECK-NEXT:    lwa 3, 4(3)
+# CHECK-NEXT:    blr
+
+
+  .globl  VarLabelSingleInsnBetween
+  .p2align  4
+  .type VarLabelSingleInsnBetween,@function
+VarLabelSingleInsnBetween:
+.LVarLabelSingleInsnBetween$local:
+  pld 3, vec@got@pcrel(0), 1
+.Lpcrel102=.-8
+        addi 3, 3, 42
+  .reloc .Lpcrel102,R_PPC64_PCREL_OPT,.-.Lpcrel102
+  lwa 3, 4(3)
+  blr
+  .long 0
+  .quad 0
+# CHECK_LABEL:   VarLabelSingleInsnBetween
+# CHECK:         pld 3, 0(0), 1
+# CHECK-NEXT:    R_PPC64_GOT_PCREL34  vec
+# CHECK-NEXT:    R_PPC64_PCREL_OPT  *ABS*+0xc
+# CHECK-NEXT:    addi 3, 3, 42
+# CHECK-NEXT:    lwa 3, 4(3)
+# CHECK-NEXT:    blr
+
+  .globl  VarLabelMultiInsnBetween                    # -- Begin function
+  .p2align  4
+  .type VarLabelMultiInsnBetween,@function
+VarLabelMultiInsnBetween:
+.LVarLabelMultiInsnBetween$local:
+  pld 3, vec@got@pcrel(0), 1
+.Lpcrel103=.-8
+        addi 3, 3, 42
+        addi 3, 3, 42
+        addi 3, 3, 42
+        addi 3, 3, 42
+        addi 3, 3, 42
+  .reloc .Lpcrel103,R_PPC64_PCREL_OPT,.-.Lpcrel103
+  lwa 3, 4(3)
+  blr
+  .long 0
+  .quad 0
+# CHECK_LABEL:   VarLabelMultiInsnBetween
+# CHECK:         pld 3, 0(0), 1
+# CHECK-NEXT:    R_PPC64_GOT_PCREL34  vec
+# CHECK-NEXT:    R_PPC64_PCREL_OPT  *ABS*+0x1c
+# CHECK-NEXT:    addi 3, 3, 42
+# CHECK-NEXT:    addi 3, 3, 42
+# CHECK-NEXT:    addi 3, 3, 42
+# CHECK-NEXT:    addi 3, 3, 42
+# CHECK-NEXT:    addi 3, 3, 42
+# CHECK-NEXT:    lwa 3, 4(3)
+# CHECK-NEXT:    blr
+
+
+  .globl  VarLabelPrefixInsnBetween                    # -- Begin function
+  .p2align  4
+  .type VarLabelPrefixInsnBetween,@function
+VarLabelPrefixInsnBetween:
+.LVarLabelPrefixInsnBetween$local:
+  pld 3, vec@got@pcrel(0), 1
+.Lpcrel104=.-8
+        addi 3, 3, 42
+        paddi 3, 3, 42, 0
+        addi 3, 3, 42
+        paddi 3, 3, 42, 0
+        addi 3, 3, 42
+  .reloc .Lpcrel104,R_PPC64_PCREL_OPT,.-.Lpcrel104
+  lwa 3, 4(3)
+  blr
+  .long 0
+  .quad 0
+# CHECK_LABEL:   VarLabelPrefixInsnBetween
+# CHECK:         pld 3, 0(0), 1
+# CHECK-NEXT:    R_PPC64_GOT_PCREL34  vec
+# CHECK-NEXT:    R_PPC64_PCREL_OPT  *ABS*+0x24
+# CHECK-NEXT:    addi 3, 3, 42
+# CHECK-NEXT:    paddi 3, 3, 42, 0
+# CHECK-NEXT:    addi 3, 3, 42
+# CHECK-NEXT:    paddi 3, 3, 42, 0
+# CHECK-NEXT:    addi 3, 3, 42
+# CHECK-NEXT:    lwa 3, 4(3)
+# CHECK-NEXT:    blr
+
+
+  .globl  VarLabelSpaceBetween                    # -- Begin function
+  .p2align  4
+  .type VarLabelSpaceBetween,@function
+VarLabelSpaceBetween:
+.LVarLabelSpaceBetween$local:
+  pld 3, vec@got@pcrel(0), 1
+.Lpcrel105=.-8
+        addi 3, 3, 42
+        paddi 3, 3, 42, 0
+        addi 3, 3, 42
+        .space 40, 0
+        paddi 3, 3, 42, 0
+        addi 3, 3, 42
+  .reloc .Lpcrel105,R_PPC64_PCREL_OPT,.-.Lpcrel105
+  lwa 3, 4(3)
+  blr
+  .long 0
+  .quad 0
+# CHECK_LABEL:   VarLabelSpaceBetween
+# CHECK:         pld 3, 0(0), 1
+# CHECK-NEXT:    R_PPC64_GOT_PCREL34  vec
+# CHECK-NEXT:    R_PPC64_PCREL_OPT  *ABS*+0x4c
+# CHECK-NEXT:    addi 3, 3, 42
+# CHECK-NEXT:    paddi 3, 3, 42, 0
+# CHECK-NEXT:    addi 3, 3, 42
+# CHECK:         paddi 3, 3, 42, 0
+# CHECK-NEXT:    addi 3, 3, 42
+# CHECK-NEXT:    lwa 3, 4(3)
+# CHECK-NEXT:    blr
+
+
+  .globl  VarLabelPlus
+  .p2align  4
+  .type VarLabelPlus,@function
+VarLabelPlus:
+.LVarLabelPlus$local:
+.Lpcrel106:
+        addi 3, 3, 42
+        addi 3, 3, 42
+  pld 3, vec@got@pcrel(0), 1
+  .reloc .Lpcrel106+8,R_PPC64_PCREL_OPT,.-(.Lpcrel106+8)
+  lwa 3, 4(3)
+  blr
+  .long 0
+  .quad 0
+# CHECK-LABEL:   VarLabelPlus
+# CHECK:         pld 3, 0(0), 1
+# CHECK-NEXT:    R_PPC64_PCREL_OPT  *ABS*+0x8
+# CHECK-NEXT:    R_PPC64_GOT_PCREL34  vec
+# CHECK-NEXT:    lwa 3, 4(3)
+# CHECK-NEXT:    blr
+
+# Check the situation where the PLD requires an alignment nop.
+  .globl  AlignPLD
+  .p2align  6
+  .type AlignPLD,@function
+        .space          60      # Force the pld to require an alignment nop.
+AlignPLD:
+.LAlignPLD$local:
+  pld 3, vec@got@pcrel(0), 1
+.Lpcrel201:
+  .reloc .Lpcrel201-8,R_PPC64_PCREL_OPT,.-(.Lpcrel201-8)
+  lwa 3, 4(3)
+  blr
+# CHECK-LABEL:   AlignPLD
+# CHECK:         nop
+# CHECK-NEXT:    pld 3, 0(0), 1
+# CHECK-NEXT:    R_PPC64_GOT_PCREL34  vec
+# CHECK-NEXT:    R_PPC64_PCREL_OPT  *ABS*+0x8
+# CHECK-NEXT:    lwa 3, 4(3)
+# CHECK-NEXT:    blr
+
+# The label and the pld are on the same line and so the nop is inserted before
+# the label and the relocation should work.
+  .globl  AlignPLDSameLine
+  .p2align  6
+  .type AlignPLDSameLine,@function
+        .space          60      # Force the pld to require an alignment nop.
+AlignPLDSameLine:
+.LAlignPLDSameLine$local:
+.Lpcrel202: pld 3, vec@got@pcrel(0), 1
+  .reloc .Lpcrel202,R_PPC64_PCREL_OPT,.-.Lpcrel202
+  lwa 3, 4(3)
+  blr
+# CHECK-LABEL:   AlignPLDSameLine
+# CHECK:         nop
+# CHECK-NEXT:    pld 3, 0(0), 1
+# CHECK-NEXT:    R_PPC64_GOT_PCREL34  vec
+# CHECK-NEXT:    R_PPC64_PCREL_OPT  *ABS*+0x8
+# CHECK-NEXT:    lwa 3, 4(3)
+# CHECK-NEXT:    blr
+
+  .globl  AlignPLDLabelBefore
+  .p2align  6
+  .type AlignPLDLabelBefore,@function
+        .space          60      # Force the pld to require an alignment nop.
+AlignPLDLabelBefore:
+.LAlignPLDLabelBefore$local:
+.Label:
+  pld 3, vec@got@pcrel(0), 1
+.Lpcrel203:
+  .reloc .Lpcrel203-8,R_PPC64_PCREL_OPT,.-(.Lpcrel203-8)
+  lwa 3, 4(3)
+  blr
+# CHECK-LABEL:   AlignPLDLabelBefore
+# CHECK:         nop
+# CHECK-NEXT:    pld 3, 0(0), 1
+# CHECK-NEXT:    R_PPC64_GOT_PCREL34  vec
+# CHECK-NEXT:    R_PPC64_PCREL_OPT  *ABS*+0x8
+# CHECK-NEXT:    lwa 3, 4(3)
+# CHECK-NEXT:    blr
+
+  .globl  AlignPLDLabelSameLine
+  .p2align  6
+  .type AlignPLDLabelSameLine,@function
+        .space          60      # Force the pld to require an alignment nop.
+AlignPLDLabelSameLine:
+.Label2: pld 3, vec@got@pcrel(0), 1
+.Lpcrel204:
+  .reloc .Lpcrel204-8,R_PPC64_PCREL_OPT,.-(.Lpcrel204-8)
+  lwa 3, 4(3)
+  blr
+# CHECK-LABEL:   AlignPLDLabelSameLine
+# CHECK:         nop
+# CHECK-NEXT:    pld 3, 0(0), 1
+# CHECK-NEXT:    R_PPC64_GOT_PCREL34  vec
+# CHECK-NEXT:    R_PPC64_PCREL_OPT  *ABS*+0x8
+# CHECK-NEXT:    lwa 3, 4(3)
+# CHECK-NEXT:    blr