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

Tue Dec 3 07:09:13 PST 2019

Xiangling_L added a comment.

I couldn't apply your patch on latest master branch, do you mind updating it?

================
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))
----------------
sfertile wrote:
> 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.
Thank you for your clarification. I  also think `!Subtarget.usesFunctionDescriptors() && !Subtarget.isELFv2ABI()` is better, it's more descriptive.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70126