[PATCH] D58378: [PowerPC]Leverage the addend in the TOC relocation to do the address calculation

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:6472
+        // LFD z, GA{off1+off2}(x)
+        Offset += GA->getOffset();
+
----------------
It is not clear to me how we ensure `Offset` fits into a signed 16-bit immediate.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:14506
 
+SDValue PPCTargetLowering::combineADDOnTOCEntry(SDNode *N, SelectionDAG &DAG) const {
+  // The addend in the TOC relocation isn't supported by all platform.
----------------
Line too long. If you are using Vim, you can run clang-format within the editor with something like:
`:14506,14560 !clang-format`
as long as you have `clang-format` in your `$PATH`.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:14507
+SDValue PPCTargetLowering::combineADDOnTOCEntry(SDNode *N, SelectionDAG &DAG) const {
+  // The addend in the TOC relocation isn't supported by all platform.
+  if (!Subtarget.isELFv2ABI())
----------------
s/platform/platforms


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:14521
+  if (!Offset) {
+    Offset = dyn_cast<ConstantSDNode>(Op0);
+    TocEntry = dyn_cast<MemIntrinsicSDNode>(Op1);
----------------
Is this actually needed? There is not canonical form (with the constant being the second operand)?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:14558
+                                               Addr->getTargetFlags());
+  bool Is64Bit = TocEntry->getMemoryVT() == MVT::i64;
+  return getTOCEntry(DAG, SDLoc(TocEntry), Is64Bit, NewAddr);
----------------
Why do we need this? Wouldn't this always be the case? The ELFv2 ABI has no support for 32-bit addressing so why is it that we need this? Could it not just be an assert?


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

https://reviews.llvm.org/D58378