[PATCH] D79864: [PowerPC] Add linker opt for PC Relative GOT indirect accesses

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCELFStreamer.cpp:98
+
+  // User of the GOT-indirect address.
+  if (IsPartOfGOTToPCRelPair.hasValue() && !IsPartOfGOTToPCRelPair.getValue())
----------------
```
// For example, the load that will get the relocation as follows:
// .reloc .Lpcrel1-8,R_PPC64_PCREL_OPT,.-(.Lpcrel1-8)
//  lwa 3, 4(3)
```


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCELFStreamer.cpp:109
+
+  // Producer of the GOT-indirect address.
+  if (IsPartOfGOTToPCRelPair.hasValue() && IsPartOfGOTToPCRelPair.getValue())
----------------
```
// For example, the prefixed load from the got that will get the label as follows:
//  pld 3, vec at got@pcrel(0), 1
// .Lpcrel1:
```


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCInstPrinter.cpp:110
+          printInstruction(MI, Address, O);
+          O << "\n";
+          O << SymbolName;
----------------
Is there not something like `printLabel()` and `printRelocDirective()` for this? It seems odd to be manually printing it like this.
If there isn't that's fine, just seems odd.


================
Comment at: llvm/lib/Target/PowerPC/PPCPreEmitPeephole.cpp:246
+        Register DefReg;
+        Register UseInstDef;
+        bool StillValid;
----------------
I know this is based on my suggestion, but I think the name doesn't make sense if this is a store (since a store does not define anything). Perhaps just `UseReg` is fine.


================
Comment at: llvm/lib/Target/PowerPC/PPCPreEmitPeephole.cpp:255
+      // collect potential pairs of GOT indirect access instructions.
+      for (auto BBI = MBB.instr_begin(); BBI != MBB.instr_end(); ++BBI) {
+        // Look for the initial GOT indirect load.
----------------
I think a simpler implementation for lines 255-311 would be something like this:
```
  for (auto BBI = MBB.instr_begin(); BBI != MBB.instr_end(); ++BBI) {
    // Look for the initial GOT indirect load.
    if (isGOTPLDpc(*BBI)) {
      GOTDefUsePair CurrentPair{BBI, MachineBasicBlock::iterator(),
                                BBI->getOperand(0).getReg(), PPC::NoRegister,
                                true};
      CandPairs.push_back(CurrentPair);
      continue;
    }

    // We haven't encountered any new PLD instructions, nothing to check.
    if (CandPairs.empty())
      continue;

    // Run through the candidate pairs and see if any of the registers
    // defined in the PLD instructions are used by this instruction.
    // Note: the size of CandPairs can change in the loop.
    for (unsigned Idx = 0; Idx < CandPairs.size(); Idx++) {
      GOTDefUsePair &Pair = CandPairs[Idx];
      // The instruction does not use or modify this PLD's def reg, ignore it.
      if (!BBI->readsRegister(Pair.DefReg, TRI) &&
          !BBI->modifiesRegister(Pair.DefReg, TRI))
        continue;

      // The use needs to be used in the address compuation and not
      // as the register being stored for a store.
      const MachineOperand *UseOp =
          hasPCRelativeForm(*BBI) ? &BBI->getOperand(2) : nullptr;

      // Check for a valid use.
      if (UseOp && UseOp->isReg() && UseOp->getReg() == Pair.DefReg &&
          UseOp->isUse() && UseOp->isKill()) {
        Pair.UseInst = BBI;
        Pair.UseInstDef = BBI->getOperand(0).getReg();
        ValidPairs.push_back(Pair);
      }
      CandPairs.erase(CandPairs.begin() + Idx);
    }
  }

  // Go through all of the pairs and check for any more valid uses.
  for (auto Pair = ValidPairs.begin(); Pair != ValidPairs.end(); Pair++) {
    // We shouldn't be here if we don't have a valid pair.
    assert(Pair->UseInst.isValid() && Pair->StillValid &&
           "Kept an invalid def/use pair for GOT PCRel opt");
```

The idea is that after the loop over the basic block, we just have a data structure containing pairs of (PLD, MemAccess) where the register defined by PLD is guaranteed to not have uses in between them. Then for each such pair, we check for defs/uses of the register defined by the MemAccess and we're done.


================
Comment at: llvm/lib/Target/PowerPC/PPCPreEmitPeephole.cpp:268
+        // If this instruction is not a PLDpc as above and the vector is still
+        // empty then there is no point in going futher. Find the next
+        // instruction.
----------------
s/futher/further


================
Comment at: llvm/test/CodeGen/PowerPC/pcrel-linkeropt.ll:159
+; CHECK-NEXT:    mtfprwz f1, r4
+; CHECK-NEXT:    lxvx vs0, 0, r3
+; CHECK-NEXT:    pld r3, outputVi32 at got@pcrel(0), 1
----------------
```
; FIXME: we should always convert X-Form instructions that use
; PPC::ZERO[8] to the corresponding D-Form so we can perform this opt.
```


================
Comment at: llvm/test/CodeGen/PowerPC/pcrel-linkeropt.ll:390
+  ret i32* @input32
+}
----------------
Please add `attributes #0 = { nounwind }` and decorate the functions with it so we don't get the `.cfi` directives since they just make the test case more busy.


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

https://reviews.llvm.org/D79864