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

[Sergei Kachkov via Phabricator via llvm-commits]

kachkov98 marked 8 inline comments as done and 2 inline comments as done.
kachkov98 added inline comments.


================
Comment at: llvm/include/llvm/Analysis/PHITransAddr.h:45
+private:
+  std::variant<Value *, SelectAddrs> Addr;
+};
----------------
kachkov98 wrote:
> nikic wrote:
> > Unless std::variant is smarter than I think, you probably want to use a pair of Values instead, and determine the variant based on whether the second is nullptr. Otherwise you'll store a redundant type tag.
> Thank you for the idea! Now we are checking that both select addresses != nullptr a bit late, but after moving this check earlier this can be implemented without tag.
New implementation is using just pair of Values.


================
Comment at: llvm/include/llvm/Analysis/PHITransAddr.h:121
+                                            BasicBlock *PredBB,
+                                            const DominatorTree *DT,
+                                            SelectInst *Sel);
----------------
mkazantsev wrote:
>  `const DominatorTree &DT` or it can be null?
Just like with translateValue(BasicBlock *, BasicBlock *, const DominatorTree *, bool) overload, DominatorTree is optional parameter. It looks like we always pass it though, but leaving it as pointer gives more flexibility.


================
Comment at: llvm/lib/Analysis/PHITransAddr.cpp:171
+      if (Sel && V == Sel)
+        return AddAsInput(Cond ? Sel->getTrueValue() : Sel->getFalseValue());
+      return AddAsInput(V);
----------------
mkazantsev wrote:
> 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?
Refactored this part: now we pass select condition and its value (true or false), so we can translate address even if it depends from multiple selects with same condition.


================
Comment at: llvm/lib/Analysis/PHITransAddr.cpp:347
+  Value *FalseAddr = PHITransAddr(*this).PHITranslateSubExpr(
+      Addr, CurBB, PredBB, DT, Sel, false);
+  return {TrueAddr, FalseAddr};
----------------
mkazantsev wrote:
> mkazantsev wrote:
> > Why not
> > ```
> >   Value *TrueAddr = PHITransAddr(*this).PHITranslateSubExpr(Addr, CurBB, PredBB,
> >                                                             DT, Sel->getTrueValue());
> >   Value *FalseAddr = PHITransAddr(*this).PHITranslateSubExpr(
> >       Addr, CurBB, PredBB, DT, Sel->getFalseValue());
> > ```
> > ?
> Also, is `PHITransAddr(*this)` twice necessary?
Unfortunately, it's required: PHITransSubExpr modifies InstInputs during its work, so we can't run this method twice for the same object.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142705