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

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added a comment.

The changes that are needed are fairly minor but I would like to take another look. I promise to get to it immediately when you have the updated patch posted.



================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp:242
+          ".localentry expression is not a valid power of 2.");
+    case 0:
+      return 0;
----------------
I believe the cases can be rewritten more concisely as:
```
case 0:
case 1:
case 4:
case 8:
case 16:
case 32:
case 64:
  return Log2(Offset) << ELF::STO_PPC64_LOCAL_BIT;
```
That way we still only allow the expected values and we have a nice simple expression for what we return.


================
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())
----------------
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)?


================
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);
----------------
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.


================
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.
----------------
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).
```


================
Comment at: llvm/lib/Target/PowerPC/PPCMIPeephole.cpp:135
+    initialize(MF);
+    unskipableSimplifyCode();
     if (skipFunction(MF.getFunction()))
----------------
Please add the following comment:
```
// FIXME: This introduces another complete traversal of the instructions
// in the function in the common case (function is not skipped). Although
// this is less than ideal for compile time, this code will go away once
// our PC-Rel implementation is complete.
```


================
Comment at: llvm/test/MC/PowerPC/ppc64-localentry-error1.s:10
 
-# CHECK: LLVM ERROR: .localentry expression cannot be encoded.
+# CHECK: LLVM ERROR: .localentry expression is not a valid power of 2.
 
----------------
efriedma wrote:
> Only loosely related to this patch, but using report_fatal_error isn't user-friendly.  We should print a proper parse error using reportError or something like that, if a user writes `.localentry foo, 33`
@efriedma Has this been adequately addressed above? The message (and termination without producing an object file) replicates the behaviour of the system assembler.


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

https://reviews.llvm.org/D73664