[PATCH] D79625: [PowerPC] Extend .reloc directive on PowerPC

Tue Jun 23 12:21:34 PDT 2020

stefanp marked 8 inline comments as done.
stefanp added inline comments.

================
Comment at: llvm/include/llvm/MC/MCExpr.h:305
     VK_PPC_NOTOC,           // symbol at notoc
+    VK_PPC_LINKER_OPT,      // Not actually emitted as symbol at ppclinkeropt

----------------
sfertile wrote:
> 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? 
I agree that the name with PCREL will work better.
I'm going to use `VK_PPC_PCREL_OPT`.

================
Comment at: llvm/lib/MC/MCExpr.cpp:325
   case VK_PPC_NOTOC: return "notoc";
+  case VK_PPC_LINKER_OPT: return "ppclinkeropt";
   case VK_COFF_IMGREL32: return "IMGREL";
----------------
nemanjai wrote:
> stefanp wrote:
> > amyk wrote:
> > > 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?
> > 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 at 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.
> 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?
No, there is nowhere else that I can think of that would require this name. 
I agree that `<<Invalid>>` is a better choice.

================
Comment at: llvm/lib/MC/MCExpr.cpp:442
     .Case("notoc", VK_PPC_NOTOC)
+    .Case("ppclinkeropt", VK_PPC_LINKER_OPT)
     .Case("gdgot", VK_Hexagon_GD_GOT)
----------------
nemanjai wrote:
> 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.
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.

================
Comment at: llvm/lib/Target/PowerPC/AsmParser/PPCAsmParser.cpp:1811
+/// ParseDirectiveReloc
+/// Very similar to the one in AsmParser::parseDirectiveReloc except that we
+/// want to be able to parse MCExpr::Binary too.
----------------
sfertile wrote:
> 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
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.

================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCELFStreamer.cpp:153
+  // If there is no MCExpr let the parent class handle it.
+  if (!Expr)
+    return MCELFStreamer::emitRelocDirective(Offset, Name, Expr, Loc, STI);
----------------
nemanjai wrote:
> 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");
> ```
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.

================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCELFStreamer.cpp:176
+
+    assert(DF && "Expected a valid data fragment.");
+
----------------
nemanjai wrote:
> I think Anil's question makes sense. Is it possible for `getOffsetFromBinaryExpr()` to return `true` and not set `DF`?
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.

================
Comment at: llvm/test/MC/PowerPC/future-reloc-with-expr.s:107
+# CHECK-NEXT:    addi 3, 3, 42
+# CHECK-NEXT:    nop
+# CHECK-NEXT:    paddi 3, 3, 42, 0
----------------
nemanjai wrote:
> What is this `nop`? Is it to align the `paddi` below? If so, it seems to me that it is in the wrong place.
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.

================
Comment at: llvm/test/MC/PowerPC/future-reloc-with-expr.s:199
+	.type	VarLabelSingleInsnBetween, at function
+VarLabelSingleInsnBetween:
+.LVarLabelSingleInsnBetween$local:
----------------
nemanjai wrote:
> 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.
That's because I forgot to change these ones to look like the test just above. I'll fix that.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79625/new/

https://reviews.llvm.org/D79625