[PATCH] D70126: [PowerPC] Refactor FinishCall

[Sean Fertile via Phabricator via llvm-commits]

sfertile marked 2 inline comments as done.
sfertile added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5029
+  // need to jump to the local entry point (see rL211174).
+  if (!Subtarget.usesFunctionDescriptors() && !Subtarget.isELFv2ABI() &&
+      isBLACompatibleAddress(Callee, DAG))
----------------
Xiangling_L wrote:
> It looks like `!Subtarget.usesFunctionDescriptors() && !Subtarget.isELFv2ABI() ` eqauls to `isDarwin() || is32ELFv1ABI()`, but AFAIK Darwin is abondoned, and there is no 32bit ELFv1 ABI(Correct me if I am wrong), can we skip this query?
There is only a single 32-bit ELF ABI so we don't need to disambiguate it by calling it v1.The current condition is equivalent to `isDarwin() || is32BitELFABI()`. Targeting Darwin is deprecated, but we haven't started ripping out the Darwin code yet so condensing to just `is32BitELFABI()` is a bit premature in my opinion. I used the form above to try to highlight that the specific ABIs have features which are incompatable with a BLA  but I think the comment here and the description in rL2111174 convey that accurately so we can switch to your suggested ` isDarwin() || is32BitELFABI()` if you feel strongly.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5355
+                             Glue, dl);
+  Glue = Chain.getValue(1);
 
----------------
Xiangling_L wrote:
> Just a question, how did you verify that `Ins` will never be empty?
`Ins` can be empty. I found conditionally updating the Glue was confusing. It made me think we glue Ins to the Call_SEQEND node, but glue something else to the Call instruction if we have no ins. From your comment it seems you assumed similarly. (We already glue the Call instr to the Call_SEQEND node and IIUC correctly gluing another node to it wouldn't be valid .) 

The reality is if we have no `Ins` then the array of return value  `CCValAssign`s in `LowerCallResult` will be empty, and there is nothing else to glue into the chain, making updating the glue harmless.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70126