[PATCH] D70126: [PowerPC] Refactor FinishCall [NFC]

Wed Nov 27 17:03:33 PST 2019

sfertile marked 3 inline comments as done.
sfertile added a comment.

In D70126#1759491 <https://reviews.llvm.org/D70126#1759491>, @Xiangling_L wrote:

> Since this is a NFC patch, does it make sense to add `NFC` on patch title?

I missed that, updated.

In D70126#1759509 <https://reviews.llvm.org/D70126#1759509>, @hubert.reinterpretcast wrote:

> If the `clang-format` "TODO" in the patch description has been addressed, then the patch description should be updated.

Done.

================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5068
+    return callsShareTOCBase(&Caller, Callee, TM) ? PPCISD::CALL
+                                                 : PPCISD::CALL_NOP;
+
----------------
Xiangling_L wrote:
> Please fix the formatting issue here.
Should be fixed now.

================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5145
+static void prepareDescriptorIndirectCall(SelectionDAG &DAG, SDValue &Callee,
+                                        SDValue &Glue, SDValue &Chain,
+                                        SDValue CallSeqStart,
----------------
Xiangling_L wrote:
> Please fix the formatting issue from line 5145 - line 5149.
Should be fixed now.

================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5249
+      SDValue AddTOC = DAG.getNode(ISD::ADD, dl, MVT::i64, StackPtr, TOCOff);
+      Ops.push_back(AddTOC);
+    }
----------------
Xiangling_L wrote:
> I feel it's worth mentioning  in the comment `The address needs to go after the chain input but before the flag (or any other variadic arguments).` for `AddTOC` like the original comment does, in case ppl mistakenly move this around in the future.
Good point. I added it back but reworded it a bit and merged it with the other comment.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70126