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

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added inline 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
----------------
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;
```


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:510
     return GetBlockAddressSymbol(MO.getBlockAddress());
+  case MachineOperand::MO_MCSymbol:
+    return MO.getMCSymbol();
----------------
It is not clear how these additions are related to what this patch does.


================
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,
----------------
What does the name signify? Is the symbol for the callee function TOC Pointer relative? I would imagine it is not.


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


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:1765
+PPCAIXAsmPrinter::getMCSymbolForTOCPseudoMO(const MachineOperand &MO,
+					    MCContext &OutContext) {
   const GlobalObject *GO = nullptr;
----------------
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).


================
Comment at: llvm/lib/Target/PowerPC/PPCMIPeephole.cpp:825
       }
+      case PPC::BL8_NOTOC: {
+        // At this point the BL8_NOTOC instruction is not really safe because it
----------------
I don't think this is the right place for this code. Please note that the enclosing function will only run if `skipFunction(MF.getFunction())` is `false`. Since this is required for correctness rather than optimization, it should be added in a place where it cannot be skipped.


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

https://reviews.llvm.org/D73664