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

[Qing Shan Zhang via Phabricator via llvm-commits]

steven.zhang marked 3 inline comments as done.
steven.zhang added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:6472
+        // LFD z, GA{off1+off2}(x)
+        Offset += GA->getOffset();
+
----------------
nemanjai wrote:
> It is not clear to me how we ensure `Offset` fits into a signed 16-bit immediate.
We don't need to ensure the Offset fits the 16-bit imm as it is the offset of the Global Address.


================
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.
----------------
nemanjai wrote:
> 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`.
Well, I will set up my IDE to avoid the format issue happening again.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:14558
+                                               Addr->getTargetFlags());
+  bool Is64Bit = TocEntry->getMemoryVT() == MVT::i64;
+  return getTOCEntry(DAG, SDLoc(TocEntry), Is64Bit, NewAddr);
----------------
nemanjai wrote:
> 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?
You are right, the ABI imply the 64 bit. Thank you.


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

https://reviews.llvm.org/D58378