[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:93
+  // Special handling for a linker optimization.
+  MCSymbol *LabelSym = checkLinkerOpt(Inst, STI);
+
----------------
I think it would be clear if at the top level here we have something like this:
```
// If the instruction is a part of the GOT to PC-Rel link time optimization
// instruction pair, return a value, otherwise return None. A true returned
// value means the instruction is the PLDpc and a false value means it is
// the user instruction.
Optional<bool> IsPartOfGOTToPCRelPair = isPartOfGOTToPCRelPair(Inst, STI);

// User of the GOT-indirect address.
if (IsPartOfGOTToPCRelPair.hasValue() && !IsPartOfGOTToPCRelPair.getValue())
  emitGOTToPCRelReloc(Inst);

// existing code to emit the instruction

// Producer of the GOT-indirect address.
if (IsPartOfGOTToPCRelPair.hasValue() && IsPartOfGOTToPCRelPair.getValue())
  emitGOTToPCRelLabel(Inst);
```


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCELFStreamer.cpp:222
 
+MCSymbol *PPCELFStreamer::checkLinkerOpt(const MCInst &Inst,
+                                         const MCSubtargetInfo &STI) {
----------------
This needs to have both a more descriptive name and a comment describing what it does. A few requests here:
1. Functions whose names indicate they are queries (i.e. `check...` etc.) should probably not modify code. Either check something or modify something.
2. Avoid the very generic phrase "linker opt{imization}" throughout this code. You should pick a name for this optimization and use it throughout. Perhaps a name such as "GOT to PC-Rel relaxation" or "Link time GOT to PC-Rel optimization".
3. One or two examples of the kind of `MCInst` for which this will return a non-null symbol.


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCELFStreamer.cpp:262
+//   load <loadedreg>, 0(<reg>)
+// The reason we use put the label after the pld instruction is because it is
+// possible to have a nop inserted between the label and the prefixed pld.
----------------
```
// The reason we place the label after the PLDpc instruction is that there
// may be an alignment nop before it since prefixed instructions must not
// cross a 64-byte boundary (please see
// PPCELFStreamer::emitPrefixedInstruction()). When referring to the
// label, we subtract the width of a prefixed instruction (8 bytes) to ensure
// we refer to the PLDpc.
```


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCELFStreamer.cpp:275
+      MCBinaryExpr::createSub(LabelExpr, Eight, getContext());
+  MCSymbol *TmpLabel = getContext().createTempSymbol();
+  const MCExpr *TmpExpr = MCSymbolRefExpr::create(TmpLabel, getContext());
----------------
`TmpLabel` is just the current location, right? Please name it as such if so.


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCInstPrinter.cpp:99
+  unsigned LastOp = MI->getNumOperands() - 1;
+  if (MI->getNumOperands() > 1) {
+    const MCOperand &Operand = MI->getOperand(LastOp);
----------------
Could we maybe pull out `isPartOfGOTToPCRelPair()` I mentioned above and use it here as well as in the streamer code?


================
Comment at: llvm/lib/Target/PowerPC/PPCPreEmitPeephole.cpp:215
 
+    bool addLinkerOpt(MachineBasicBlock &MBB, const TargetRegisterInfo *TRI) {
+      bool MadeChange = false;
----------------
The algorithm here is a bit hard to follow by just reading the code. We search forward looking for a `PLDpc`. When we find one, we search forward for a user. Once we find the user, if the user defines any registers, we search back to make sure there are no intervening uses or defs of that register.

It might be easier to follow if we do a single pass to collect all address defs and uses and then check code in between for any candidate pairs.
Something along these lines:

```
struct GOTDefUsePair {
  MachineBasicBlock::iterator DefInst;
  MachineBasicBlock::iterator UseInst;
  Register DefReg;
  Register UseInstDef;
};

SmallVector<GOTDefUsePair> Pairs;
loop over the block
  if (instruction is a candidate PLDpc)
    create a pair object and push it to Pairs
    continue
  if (instruction is a use of any defs in Pairs)
    if (not a kill)
      remove pair object from Pairs (this can also be done by keeping a Valid flag for each pair)
      continue
    add the use iterator and register (if any) to the pair in question
```

Then after collecting instruction pairs, loop over the vector to eliminate invalid pairs (due to register uses/defs, etc.) and finally add the symbols.


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

https://reviews.llvm.org/D79864