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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCELFStreamer.cpp:205
+MCSymbol *PPCELFStreamer::checkLinkerOpt(const MCInst &Inst,
+                                         const MCSubtargetInfo &STI) {
+  MCSymbol *LabelSym = nullptr;
----------------
The way this is structured is a little confusing.  If the instruction is a pld, it returns the label; if it's some other instruction, it emits a relocation itself.  It would probably be more clear to split the "emit a relocation" part into a separate function.


================
Comment at: llvm/lib/Target/PowerPC/MCTargetDesc/PPCELFStreamer.cpp:230
+  // fixup can be added.
+  LabelSym = getContext().getOrCreateSymbol(SymExpr->getSymbol().getName());
+  if (Inst.getOpcode() == PPC::PLDpc)
----------------
Isn't `getContext().getOrCreateSymbol(SymExpr->getSymbol().getName());` equivalent to `SymExpr->getSymbol()`?


================
Comment at: llvm/lib/Target/PowerPC/PPCPreEmitPeephole.cpp:264
+          if (SearchBBI->readsRegister(FirstOp.getReg(), TRI) ||
+              SearchBBI->modifiesRegister(FirstOp.getReg(), TRI)) {
+            IsStillValid = false;
----------------
To be on the safe side, I think I'd add an implicit def of the register to the pld, and an implicit use to the load, so it's clear to any later passes that the register is actually live between the pld and the load


================
Comment at: llvm/lib/Target/PowerPC/PPCPreEmitPeephole.cpp:271
+        if (IsStillValid) {
+          static uint64_t UseCounter = 0;
+          UseCounter++;
----------------
Storing a counter in a static variable isn't threadsafe or deterministic.  Please store the counter in PPCFunctionInfo or something like that.

Or actually, in this context, you could probably use createTempSymbol, which manages the counter for you.


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

https://reviews.llvm.org/D79864