[PATCH] D34245: [PowerPC] Refine the checking for emiting TOC restore nops and Tail-Call eligibility.

Nemanja Ivanovic via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Jun 21 05:24:54 PDT 2017


nemanjai added inline comments.


================
Comment at: lib/Target/PowerPC/PPCISelLowering.cpp:4254
+static bool
+shouldAssumeDifferentTOC(const Function *Caller, SDValue Callee,
+                                     const TargetMachine &TM) {
----------------
sfertile wrote:
> gyiu wrote:
> > Should we just move the code from this function to the existing resideInSameSection() function and rename the function itself?  I see in there we're also checking 
> > 
> >     GlobalAddressSDNode *G = dyn_cast<GlobalAddressSDNode>(Callee);
> >     if (!G)
> >       return true;
> I think that is a good suggestion. I can't think of any other reason we would want to see if 2 symbols reside in the same section other then if they share a TOC base. I'll update it.
I agree. Both calls to this function are really after the same thing. Although I find this name just slightly misleading. Perhaps something like `mayHaveDifferentTOCs()` or `callRequiresTOCRestoreNop()`.


================
Comment at: test/CodeGen/PowerPC/ppc64-blnop.ll:82
+; SCM-LABEL: wo_hcaller:
+; SCM:       wo_hcallee
+; SCM-NEXT:  nop
----------------
Well, I think it would be more appropriate to include the call opcode as well - I imagine this one would be `bl`.


================
Comment at: test/CodeGen/PowerPC/ppc64-calls.ll:28
+; because they may be overridden in a different module. With large and medium
+; codemodels, no TOC restore is needed.
 define void @test_weak() nounwind readnone {
----------------
This comment is misleading. It almost seems to imply that with the large and medium code model, a weak definition can't be overridden in a different module. You should probably say something like "With the large and medium code models, both definitions will have the same TOC since they won't reside in different DSO's."

Or something along those lines.


Repository:
  rL LLVM

https://reviews.llvm.org/D34245





More information about the llvm-commits mailing list