[PATCH] D76160: [PowerPC][Future] Add offsets to PC Relative relocations.

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai accepted this revision.
nemanjai added a comment.
This revision is now accepted and ready to land.

Aside from a few minor nits, LGTM.



================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCCodeEmitter.cpp:210
+  case MCExpr::SymbolRef: {
+    // Case 1). Relocation alone.
     const MCSymbolRefExpr *SRE = cast<MCSymbolRefExpr>(Expr);
----------------
anil9 wrote:
> Generally the comments are above the code, here it seems the comments like Case1) are after the cases.
It may be confusing to mention `Case 1)`. Just describe what the case is.


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCCodeEmitter.cpp:251
+                        static_cast<MCFixupKind>(PPC::fixup_ppc_pcrel34)));
+    // Return the offset that should be added to the relocation by the linker.
+    return (CE->getValue() & 0x3FFFFFFFFUL) | RegBits;
----------------
Maybe an assert that the incoming value fits in 34 bits.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:15657
+                                          const PPCSubtarget &Subtarget) {
+  if (!Subtarget.isPPC64() || !Subtarget.hasPCRelativeMemops())
+    return SDValue();
----------------
ABI check?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:15662
+  // If we find that node try to cast the Global Address and the Constant.
+  SDValue OP0 = N->getOperand(0);
+  SDValue OP1 = N->getOperand(1);
----------------
In the other function, we use LHS/RHS. Please use that convention here as well for consistency.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:15665
+
+  if (OP0.getNode()->getOpcode() != PPCISD::MAT_PCREL_ADDR)
+    std::swap(OP0, OP1);
----------------
Please remove unnecessary calls to `getNode()`.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:15671
+
+  GlobalAddressSDNode *GSDN = dyn_cast<GlobalAddressSDNode>(OP0.getOperand(0));
+  ConstantSDNode* ConstNode = dyn_cast<ConstantSDNode>(OP1);
----------------
Just a comment above that operand zero of `MAT_PCREL_ADDR` is the GA node.


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

https://reviews.llvm.org/D76160