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

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added a comment.
Herald added a subscriber: wuzish.

I think it would be useful to add a binary test case using either `llvm-objdump` or `readelf` if that is available. Just to test the desired encoding.



================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCCodeEmitter.cpp:199
+  // 1) or 2) above. A false here is case 3) and we drop to the return at
+  // the end.
   const MCOperand &MO = MI.getOperand(OpNo);
----------------
Can we just simplify this by making it explicit with an early return?
```
if (!MO.isExpr())
  return getMachineOpValue(...)
// The rest of the code is for expressions.
```


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCCodeEmitter.cpp:231
+      const MCConstantExpr *CE = 0;
+      if (LHS->getKind() == MCExpr::SymbolRef &&
+          RHS->getKind() == MCExpr::Constant) {
----------------
Perhaps simplify this by canonicalizing to `Reloc+Offset`?
```
if (LHS->getKind != MCExpr::SymbolRef)
  std::swap(LHS, RHS);
if (LHS->getKind() != MCExpr::SymbolRef ||
    RHS->getKind() != MCExpr::Constant)
  llvm_unreachable("Expecting to have one constant and one relocation.");
const MCSymbolRefExpr *SRE = cast<MCSymbolRefExpr>(LHS);
const MCConstantExpr *CE = cast<MCConstantExpr>(RHS);
```


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:15653
+    return SDValue();
+  if (!Subtarget.hasPCRelativeMemops())
+    return SDValue();
----------------
Combine these two to a single condition.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:15662
+  ConstantSDNode* ConstNode = 0;
+  if (OP0.getNode()->getOpcode() == PPCISD::MAT_PCREL_ADDR) {
+    GSDN = dyn_cast<GlobalAddressSDNode>(OP0.getOperand(0).getNode());
----------------
Similarly to above, canonicalize to one direction.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:15673
+  // Check that both casts succeeded.
+  if (GSDN && ConstNode) {
+    int64_t NewOffset = GSDN->getOffset() + ConstNode->getSExtValue();
----------------
Flip and early exit.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:15678
+    // The signed int offset needs to fit in 34 bits.
+    APInt APOffset(64, NewOffset);
+    if (!APOffset.isSignedIntN(34))
----------------
Are you only creating an `APInt` object to check that it fits? If so, you should just use `isInt<34>(NewOffset)` and avoid using APInt altogether.


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

https://reviews.llvm.org/D76160