[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.

I really think we should add significantly more testing to this patch. We should have test cases to cover the following (we can also just add run lines with `-mcpu=future` to existing tests):

- Calls from functions that use the TOC (i.e. globals, etc.)
- Calls from functions that don't use the TOC
- Indirect calls
- Tail calls (i.e. that they are disabled)
- Handling of X2 in leaf functions (since this has changed AFAICT)



================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:507
     return GetBlockAddressSymbol(MO.getBlockAddress());
+  case MachineOperand::MO_ExternalSymbol:
+    return OutContext.getOrCreateSymbol(MO.getSymbolName());
----------------
A test case demonstrating the need for this as well as a code comment is needed here.


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:563
+  // case adds the @notoc as required.
+  case PPC::BL8_NOTOC: {
+    const MachineOperand &MO = MI->getOperand(0);
----------------
It seems kind of surprising that we do not need to do anything special here for the existing versions of `BL8/BL8_NOP/BL8_TLS/...` but we do need it for this. Can you explain why that is?


================
Comment at: llvm/lib/Target/PowerPC/PPCAsmPrinter.cpp:862
+  // so it does not need a nop after it.
+  case PPC::TAILB:
+  case PPC::TAILBCTR:
----------------
Since we are not doing anything in these cases, perhaps we should not add them in this patch but defer that until the patch that adds handling for them.


================
Comment at: llvm/lib/Target/PowerPC/PPCISelLowering.cpp:5071
 
+  if (Subtarget.isUsingPCRelativeCalls()) {
+    assert(Subtarget.is64BitELFABI() && "PC Relative is only on ELF ABI.");
----------------
So we will currently use `BCTRL + LD 2, 24(1)` for indirect calls even with PC-Relative? I suspect that the plan is for that to change in an upcoming patch and for this to be moved up before the previous check. If that is so, please add a comment to that end.


================
Comment at: llvm/lib/Target/PowerPC/PPCMIPeephole.cpp:279
+  PPCFunctionInfo *FuncInfo = MF->getInfo<PPCFunctionInfo>();
+  if (HasR2)
+    FuncInfo->setUsesTOCBasePtr();
----------------
I think we don't need to do anything in the `if (!HasR2)` case. Can we just use an early exit for that?


================
Comment at: llvm/lib/Target/PowerPC/PPCRegisterInfo.cpp:157
+  bool SaveR2 = MF->getRegInfo().isAllocatable(PPC::X2) &&
+                !Subtarget.isUsingPCRelativeCalls();
 
----------------
I initially started typing out a comment that I don't think this is safe. But then, looking through the code I convinced myself it is safe. However, I think a comment is in order because of the non-obviousness of the fact that this is safe.
Perhaps:
```
// We do not need to treat R2 as callee-saved when using PC-Relative calls
// because any direct uses of R2 will cause it to be reserved. If the function
// is a leaf or the only uses of R2 are implicit uses for calls, the calls
// will use the @notoc relocation which will cause this function to set the
// st_other bit to 1, thereby communicating to its caller that it arbitrarily
// clobbers the TOC.
```


================
Comment at: llvm/lib/Target/PowerPC/PPCSubtarget.cpp:229
+bool PPCSubtarget::isUsingPCRelativeCalls() const {
+  return isPPC64() && hasPCRelativeMemops() &&
+         CodeModel::Medium == getTargetMachine().getCodeModel();
----------------
I believe we also need to add a check for `ELFv2ABI` here since I don't think we want to do this on other ABI's, right?


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

https://reviews.llvm.org/D73664