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

Tue Feb 4 18:03:45 PST 2020

stefanp marked 9 inline comments as done.
stefanp added a comment.

Address a number of review comments.

================
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
----------------
sfertile wrote:
> 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.
I turned this into a switch statement to clean it up a little. Basically, the valid values are (0, 1, 4, 8, 16, 32, 64). We do now support a value of 1 with this patch. The new version of the function will report an error if the input Offset is not one of the acceptable values.
The reason I wanted to use a switch is because it makes which values are valid quite obvious.

================
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.");
----------------
sfertile wrote:
> 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?
Yes, we can now have Res equal to 1. We did not have support for this before but we need support for this for the implementation of PCRelative. If a function has no uses of R2 but does have function calls with the `@notoc` relocation we need to use an st_other=1 for it.

================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:510
     return GetBlockAddressSymbol(MO.getBlockAddress());
+  case MachineOperand::MO_MCSymbol:
+    return MO.getMCSymbol();
----------------
nemanjai wrote:
> It is not clear how these additions are related to what this patch does.
You are correct. These changes are not needed. I'm going to remove them.

================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:571
+    const MCSymbol *const MOSymbol = getMCSymbolForTOCPseudoMO(MO, OutContext);
+    const MCExpr *SymDtprel =
+      MCSymbolRefExpr::create(MOSymbol, MCSymbolRefExpr::VK_PPC_NOTOC,
----------------
nemanjai wrote:
> What does the name signify? Is the symbol for the callee function TOC Pointer relative? I would imagine it is not.
Copy paste mistake on my part. I've changed the name.

================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1591
       TS->emitLocalEntry(cast<MCSymbolELF>(CurrentFnSym), LocalOffsetExp);
+  } else if (Subtarget->isELFv2ABI()) {
+    // The function does not use R2 but we may still have to add the localentry
----------------
nemanjai wrote:
> I don't really understand the need for this. If the function has calls, presumably `R2` is marked as used. On the other hand, if it only has `@notoc` calls, it shouldn't need to set up `R2`. The latter is what you are adding here and the former is already handled. So under what conditions do we need global and local entry points in functions that have calls but do not mark `R2` as used?
It used to be the case that R2 was marked as used on all function calls but that is no longer true at this point. This patch removes the marking of R2 as used on calls. (See the changes to PPCISelLowering).  We have now removed the assumption that the caller needs to have a valid TOC pointer.

This `else if` will be hit in situations where a function does not need a valid TOC pointer but that function has calls to other, possibly external, functions. Basically, we only have `@notoc` calls. Since we do not know what the functions we are calling do with R2 we must assume that they clobber the register. Therefore, callers of our function need to know that this function does not preserve R2. Or, more accurately, we cannot guarantee that this function preserves R2. Therefore we need to emit a `.localentry` for this function such that the `st_other` is set to 1. (Like `MCConstantExpr::create(1, OutContext)`).  This value for `st_other` still does not set up a TOC pointer and is just used by the linker to know which functions may clobber R2. The global and local entry points remain the same.

I'm going to add a comment in the code to try to explain this as best I can.

================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1765
+PPCAIXAsmPrinter::getMCSymbolForTOCPseudoMO(const MachineOperand &MO,
+					    MCContext &OutContext) {
   const GlobalObject *GO = nullptr;
----------------
nemanjai wrote:
> The formatting looks weird. It may be right, just looks weird. In either case, it is easy to fix this ambiguity if you have your local changes committed and run:
> `git clang-format <previous hash>`
> (it will clang-format all the changes since the commit with the hash you specified).
Done.
Strangely, this line was not changed by clang-format but others were.

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

================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5385

-  if (Subtarget.is64BitELFABI() || Subtarget.isAIXABI())
+  if ((Subtarget.is64BitELFABI() || Subtarget.isAIXABI()) &&
+      !Subtarget.isUsingPCRelativeCalls())
----------------
sfertile wrote:
> Minor nit: `if ((Subtarget.is64BitELFABI() && !Subtarget.isUsingPCRelativeCalls()) || Subtarget.isAIXABI())`
Done.

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

https://reviews.llvm.org/D73664