[PATCH] D142705: [GVN] Support address translation through select instructions

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev requested changes to this revision.
mkazantsev added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/include/llvm/Analysis/MemoryDependenceAnalysis.h:237
+  NonLocalDepResult(BasicBlock *bb, MemDepResult result,
+                    const SelectAddr &address)
       : Entry(bb, result), Address(address) {}
----------------
Name convention, pls pre-commit NFC to fix it first.


================
Comment at: llvm/include/llvm/Analysis/PHITransAddr.h:32
+  using SelectAddrs = std::pair<Value *, Value *>;
+  SelectAddr(Value *V) : Addr(V){};
+  SelectAddr(const SelectAddrs &Addrs) : Addr(Addrs){};
----------------
Formatting?


================
Comment at: llvm/include/llvm/Analysis/PHITransAddr.h:89
+        if (auto *SI = dyn_cast<SelectInst>(I))
+          return SI;
+    return nullptr;
----------------
Is it possible that two instructions in `InstInputs` are in the same block? If no, you can `else return nullptr;` here.

If it is possible, you have a bug, because two of them can be selects, and what you return is kind of random?

In either case, you need some assertions to ensure prerequisites you are relying on here.


================
Comment at: llvm/include/llvm/Analysis/PHITransAddr.h:116
 
+  // SelTranslateValue - PHI translate the current address from CurBB to PredBB,
+  // and if PredBB has Sel dependency instruction, translate both cases of this
----------------
Three slashes.
Use `\p` when addressing parameters.


================
Comment at: llvm/include/llvm/Analysis/PHITransAddr.h:119
+  // select.
+  SelectAddr::SelectAddrs SelTranslateValue(BasicBlock *CurBB,
+                                            BasicBlock *PredBB,
----------------
name conventions here are widely broken, functions are supposed to be in `camelCase`.


================
Comment at: llvm/include/llvm/Analysis/PHITransAddr.h:121
+                                            BasicBlock *PredBB,
+                                            const DominatorTree *DT,
+                                            SelectInst *Sel);
----------------
 `const DominatorTree &DT` or it can be null?


================
Comment at: llvm/include/llvm/Analysis/PHITransAddr.h:143
 private:
   Value *PHITranslateSubExpr(Value *V, BasicBlock *CurBB, BasicBlock *PredBB,
+                             const DominatorTree *DT, SelectInst *Sel = nullptr,
----------------
Any explanation what are `Sel` and `Cond`?




================
Comment at: llvm/lib/Analysis/PHITransAddr.cpp:170
+      auto *V = PN->getIncomingValueForBlock(PredBB);
+      if (Sel && V == Sel)
+        return AddAsInput(Cond ? Sel->getTrueValue() : Sel->getFalseValue());
----------------
Check on `Sel != nullptr` is not needed because `V` is never null. It cannot be equal to `Sel = null`.


================
Comment at: llvm/lib/Analysis/PHITransAddr.cpp:171
+      if (Sel && V == Sel)
+        return AddAsInput(Cond ? Sel->getTrueValue() : Sel->getFalseValue());
+      return AddAsInput(V);
----------------
Why did't you just pass `Cond ? Sel->getTrueValue() : Sel->getFalseValue()` as parameter? `Cond` is not used for anything else, or I'm missing something?


================
Comment at: llvm/lib/Analysis/PHITransAddr.cpp:309
 
-
 /// PHITranslateValue - PHI translate the current address up the CFG from
----------------
stray change


================
Comment at: llvm/lib/Analysis/PHITransAddr.cpp:347
+  Value *FalseAddr = PHITransAddr(*this).PHITranslateSubExpr(
+      Addr, CurBB, PredBB, DT, Sel, false);
+  return {TrueAddr, FalseAddr};
----------------
Why not
```
  Value *TrueAddr = PHITransAddr(*this).PHITranslateSubExpr(Addr, CurBB, PredBB,
                                                            DT, Sel->getTrueValue());
  Value *FalseAddr = PHITransAddr(*this).PHITranslateSubExpr(
      Addr, CurBB, PredBB, DT, Sel->getFalseValue());
```
?


================
Comment at: llvm/lib/Transforms/Scalar/GVN.cpp:1277
+    auto [TrueAddr, FalseAddr] = Addr.getSelectAddrs();
+    assert(TrueAddr && TrueAddr->getType() == Load->getPointerOperandType() &&
+           FalseAddr && FalseAddr->getType() == Load->getPointerOperandType());
----------------
Make it separate asserts. If it fails, not clear what exactly. And also add them some description.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142705