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

Fri Jun 19 15:14:15 PDT 2020

nemanjai added inline comments.

================
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";
----------------
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?

================
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)
----------------
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.

================
Comment at: llvm/lib/Target/PowerPC/AsmParser/PPCAsmParser.cpp:1813
+/// want to be able to parse MCExpr::Binary too.
+/// Format: .reloc expression , identifier [ , expression ]
+bool PPCAsmParser::ParseDirectiveReloc(SMLoc DirectiveLoc) {
----------------
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.

================
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);
----------------
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");
```

================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCELFStreamer.cpp:156
+
+  // Get the MCFixupKind that corresponds to the string Name variable.
+  Optional<MCFixupKind> MaybeKind =
----------------
`// Get the MCFixupKind that corresponds to Name.`

================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCELFStreamer.cpp:165
+  // Handle the Binary and Symbol cases separately. Anything else just drops
+  // through for default handling in the parent class.
+  switch (Offset.getKind()) {
----------------
Please replace uses of `parent` and `parent class` with `base class`.

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

================
Comment at: llvm/test/MC/PowerPC/future-reloc-with-expr.s:2
+# RUN: llvm-mc -triple=powerpc64le-unknown-unknown -filetype=obj %s | \
+# RUN: llvm-objdump -dr --mcpu=future - | FileCheck %s
+
----------------
lei wrote:
> lei wrote:
> > Should we use `--mcpu=power10` since that's available?
> > Maybe the name of this file should also be updated? `pcRel-reloc-with-expr.s`?
> do we need to add tests for BE?
 # Please add at least some testing where the instructions in between include prefixed instr alignment `nop`s. 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`).
 # Please add a test case where the `pld` itself is being realigned
 # Please add a test case where the realigned `pld` has a label (both on the same line and on the line above)

================
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
----------------
What is this `nop`? Is it to align the `paddi` below? If so, it seems to me that it is in the wrong place.

================
Comment at: llvm/test/MC/PowerPC/future-reloc-with-expr.s:199
+	.type	VarLabelSingleInsnBetween, at function
+VarLabelSingleInsnBetween:
+.LVarLabelSingleInsnBetween$local:
----------------
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.

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

https://reviews.llvm.org/D79625