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

[Sean Fertile via Phabricator via llvm-commits]

sfertile added inline comments.


================
Comment at: llvm/lib/MC/MCExpr.cpp:325
   case VK_PPC_NOTOC: return "notoc";
+  case VK_PPC_PCREL_OPT:
+    return "<<invalid>>";
----------------
nit: Should  be on one line.


================
Comment at: llvm/lib/Target/PowerPC/AsmParser/PPCAsmParser.cpp:1814
+/// Format: .reloc expression , identifier [ , expression ]
+/// For example:
+///   pld 3, vec at got@pcrel(0), 1
----------------
Do we want the example to show up in the doxygen?


================
Comment at: llvm/lib/Target/PowerPC/AsmParser/PPCAsmParser.cpp:1819
+///   lwa 3, 4(3)
+/// In the above example the .reloc specifies to the linker that the instruction
+/// at position .Lpcrel-8 (in this case the pld) has a relocation of type
----------------
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 at 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).
```


================
Comment at: llvm/lib/Target/PowerPC/AsmParser/PPCAsmParser.cpp:1849
+                 Offset->getKind() != llvm::MCExpr::SymbolRef &&
+                 Offset->getKind() != llvm::MCExpr::Binary,
+            OffsetLoc,
----------------
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.


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCAsmBackend.cpp:72
   case PPC::fixup_ppc_pcrel34:
+  case PPC::fixup_ppc_linker_opt:
   case FK_Data_8:
----------------
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.


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCAsmBackend.cpp:105
       { "fixup_ppc_pcrel34",     0,      34,   MCFixupKindInfo::FKF_IsPCRel },
+      { "fixup_ppc_linker_opt",  0,      64,   MCFixupKindInfo::FKF_IsPCRel },
       { "fixup_ppc_nofixup",      0,      0,   0 }
----------------
Shouldn't this fixup be the same as `fixup_ppc_nofixups`?


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCAsmBackend.cpp:118
       { "fixup_ppc_pcrel34",     0,      34,   MCFixupKindInfo::FKF_IsPCRel },
+      { "fixup_ppc_linker_opt",  0,      64,   MCFixupKindInfo::FKF_IsPCRel },
       { "fixup_ppc_nofixup",     0,       0,   0 }
----------------
ditto.


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCELFStreamer.cpp:114
+  const MCExpr *RHS = BinExpr.getRHS();
+  // At this point we only support Symbol +/- Constant.
+  if (LHS->getKind() != MCExpr::SymbolRef || RHS->getKind() != MCExpr::Constant)
----------------
Drop `At this point`.


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCELFStreamer.cpp:118
+
+  // If we have reached this far the LHS is a symbol and the RHS is a
+  // constant.
----------------
Comment is redundant, its obvious from the code that LHS is a SymbolRefExpr and RHS is a ConstantExpr.


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCELFStreamer.cpp:120
+  // constant.
+  const MCSymbolRefExpr *Symbol = static_cast<const MCSymbolRefExpr *>(LHS);
+  const MCConstantExpr *ConstVal = static_cast<const MCConstantExpr *>(RHS);
----------------
Why a `static_cast` instead of a 'cast'?


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCELFStreamer.cpp:143
+
+  *DF = static_cast<MCDataFragment *>(Fragment);
+  return true;
----------------
Again why the static cast?


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCFixupKinds.h:46
 
+  // A linker opt fixup.
+  fixup_ppc_linker_opt,
----------------
```/// 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.```


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

https://reviews.llvm.org/D79625