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

[Stefan Pintilie via Phabricator via llvm-commits]

stefanp added inline comments.


================
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
----------------
sfertile wrote:
> Do we want the example to show up in the doxygen?
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.


================
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
----------------
sfertile wrote:
> 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).
> ```
I agree your description is much clearer.


================
Comment at: llvm/lib/Target/PowerPC/AsmParser/PPCAsmParser.cpp:1849
+                 Offset->getKind() != llvm::MCExpr::SymbolRef &&
+                 Offset->getKind() != llvm::MCExpr::Binary,
+            OffsetLoc,
----------------
sfertile wrote:
> 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.
Yes, we are relying on `getOffsetFromBinaryExpr` to catch the error.
However, I'll add another check in here.


================
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:
----------------
sfertile wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79625