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

[Sean Fertile via Phabricator via llvm-commits]

sfertile added inline comments.


================
Comment at: llvm/include/llvm/BinaryFormat/ELF.h:396
 }
 static inline unsigned encodePPC64LocalEntryOffset(int64_t Offset) {
   unsigned Val =
----------------
Why not move this to `PPCMCTargetDesc.cpp` since thats the only place that calls it in the monorepo.


================
Comment at: llvm/include/llvm/BinaryFormat/ELF.h:398
   unsigned Val =
       (Offset >= 4 * 4 ? (Offset >= 8 * 4 ? (Offset >= 16 * 4 ? 6 : 5) : 4)
+                       : (Offset >= 2 * 4 ? 3
----------------
nemanjai wrote:
> This ternary operator nesting is so deep that it is now unreadable. Please refactor it. Also, what is supposed to happen if `Offset` is 1, 2 or 3? Previously, we set the return value to zero, but now you are just returning that value (shifted). Is that what you want?
> 
> Perhaps something like (using `Log2` from `MathExtras.h`):
> ```
> unsigned Val = 0;
> if (Offset < 4)
>   Val = Offset;
> else
>   Val = Log2(Offset);
> Val = Val >= 6 ? 6 : Val;
> Val = Val < 0 ? 0 : Val;
> ```
There are only certain values that make sense: The 'Symbol Values' section of the ABI describes the valid values. For example 1, 2 or 3 would be nonsense. The offset from the GEP to the LEP is 0 if we don't need to setup a toc-pointer, and at least 4-bytes if we do need to setup a toc-pointer.

 +1 to Nemanjas suggestion of using Log2 to calculate the encoding. I would add a bit more error handling though: 
- Assert the offset is within the expected range of [0, 64]
- Early return for an Offset of 0.
- Assert the offset is a power of 2.
- return the log of the value.


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCTargetDesc.cpp:177
+    // indicates a an offset of 0 but indicates that r2 is preserved.
+    if (Res != 1 && Res != ELF::decodePPC64LocalEntryOffset(Encoded))
       report_fatal_error(".localentry expression cannot be encoded.");
----------------
Can `Res` be evaluated to 1? It has a very special meaning: The local and global entry points are the same, and r2 should be treated as caller-saved for callers. I'm not aware of LLVM having support for that yet but I could be mistaken. If it does already have support do we have a lit test for it?


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5064
+  if (Subtarget.isAIXABI() || Subtarget.is64BitELFABI()) {
+    if (Subtarget.isUsingPCRelativeCalls())
+      return PPCISD::CALL_NOTOC;
----------------
Minor nit: I would add a distinct conditional for this before the above comment:

```
if (Subtarget.isUsingPCRelativeCalls()) {
  assert(Subtarget.is64BitELFABI() ...)
  return PPCISD::CALL_NOTOC
}
```


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5385
 
-  if (Subtarget.is64BitELFABI() || Subtarget.isAIXABI())
+  if ((Subtarget.is64BitELFABI() || Subtarget.isAIXABI()) &&
+      !Subtarget.isUsingPCRelativeCalls())
----------------
Minor nit: `if ((Subtarget.is64BitELFABI() && !Subtarget.isUsingPCRelativeCalls()) || Subtarget.isAIXABI())`


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

https://reviews.llvm.org/D73664