[PATCH] D73664: [PowerPC][Future] Add Support For Functions That Do Not Use A TOC.

Stefan Pintilie via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Mar 5 14:49:34 PST 2020


stefanp marked 5 inline comments as done.
stefanp added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCAsmBackend.cpp:107
       { "fixup_ppc_br24",        2,      24,   MCFixupKindInfo::FKF_IsPCRel },
+      { "fixup_ppc_br24_notoc",  2,      24,   MCFixupKindInfo::FKF_IsPCRel },
       { "fixup_ppc_brcond14",    2,      14,   MCFixupKindInfo::FKF_IsPCRel },
----------------
anil9 wrote:
> Similar to the changes you made to this file, should you also be changing the function "shouldForceRelocation"
That's a good point. I'll add it.


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCELFObjectWriter.cpp:85
     case PPC::fixup_ppc_br24:
     case PPC::fixup_ppc_br24abs:
       switch (Modifier) {
----------------
anil9 wrote:
> Was wondering if you should be adding the case "case PPC::fixup_ppc_br24_notoc:" here or not.
I will add it up here and remove the case below.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:869
+    // Currently PCRelative is only supported under very specific conditions.
+    if (!Subtarget->isPPC64() || !Subtarget->isELFv2ABI() ||
+        !Subtarget->hasPCRelativeMemops())
----------------
nemanjai wrote:
> Don't we also need to make sure that the caller does not have any uses of the TOC? I believe this assumes no uses of the TOC if we have PC-Rel enabled and are on the right object mode/ABI. But aren't there still possible uses of the TOC in the function (at least for now)?
At this point all tail calls use the same TOC as the caller so it is safe to mark all of them as @notoc for now. When we expand tail calls to be able to call into other functions (as you mentioned below) we will have to add the guard. 


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:875
+    assert(MO.getType() != MachineOperand::MO_ExternalSymbol &&
+           "Extrnal symbol for tail call is unsupported.\n");
+    const MCSymbol *MOSymbol = getMCSymbolForTOCPseudoMO(MO);
----------------
nemanjai wrote:
> s/Extrnal/External
> 
> Also, is this a temporary thing? IIRC, the reason we can't tail call external functions is because they may clobber the TOC. But if we no longer promise to maintain the TOC in this function (i.e. through the `st_other` bit), can we not actually do the tail call opt?
> Please note that I am not suggesting changing this in this patch, just that we may want to add a `FIXME` explaining that this is temporary.
Yes this should be a temporary thing. I will add a FIXME comment and I will also add that we need to add a guard to make sure that the caller does not use a TOC when we start implementing this addition. 


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1588
+    // function makes calls (or is a leaf function).
+    // 1) A leaf function that does not use R2. In this case st_other=0 and both
+    //    the local and global entry points for the function are the same.
----------------
nemanjai wrote:
> Thanks for the detailed explanation here. I would just amend this slightly:
> ```
> 1) A leaf function that does not use R2 (or treats it as
>    callee-saved and preserves it).
> ```
Done.


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

https://reviews.llvm.org/D73664





More information about the llvm-commits mailing list