[PATCH] D81126: [PowerPC] Fix for PC Relative call protocol
Sean Fertile via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jun 29 09:10:17 PDT 2020
sfertile added inline comments.
================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:4723
+ // If the callee is not dso-local then they caller and callee might not share
+ // toc-bases.
----------------
I think we can pare down this comment.
The example does a good job at showing why its the callees pre-emptability that matters rather then if the caller/callee are in the same section. I'm not sure that in itself is useful enough to warrant a long comment in the code though. I think the relationship is straightforward enough:
```If the callee is preemptable, then the static linker will use a plt-stub which saves the toc to the stack, and needs a nop after the call instruction to convert to a toc-restore. ```
In this context the given example needing a toc-restore or being un-tail-callable is obvious. I think we should replace the existing comment with something similar to what I used above.
================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:4757
+ // the TOC.
+ if (!F && !Alias)
+ return false;
----------------
This check is redundant.
================
Comment at: llvm/test/CodeGen/PowerPC/func-alias.ll:4
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64le-unknown-linux-gnu \
+; RUN: -mcpu=pwr9 -ppc-asm-full-reg-names < %s | FileCheck %s --check-prefix=P9
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64le-unknown-linux-gnu \
----------------
Minor nit: I think having P9 and P10 cases is enough coverage.
================
Comment at: llvm/test/CodeGen/PowerPC/ifunc.ll:7
+; RUN: -verify-machineinstrs | FileCheck --check-prefix=LEP8 %s
+; RUN: llc %s -o - -mtriple=powerpc64le-unknown-linux-gnu -mcpu=pwr9 \
+; RUN: -verify-machineinstrs | FileCheck --check-prefix=LEP9 %s
----------------
Minor nit: I don't think we need a P9 run. P8 and P10 are enough.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D81126/new/
https://reviews.llvm.org/D81126
More information about the llvm-commits
mailing list