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

[Sergei Kachkov via Phabricator via llvm-commits]

kachkov98 added inline comments.


================
Comment at: llvm/include/llvm/Analysis/PHITransAddr.h:45
+private:
+  std::variant<Value *, SelectAddrs> Addr;
+};
----------------
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.


================
Comment at: llvm/include/llvm/Analysis/PHITransAddr.h:89
+        if (auto *SI = dyn_cast<SelectInst>(I))
+          return SI;
+    return nullptr;
----------------
mkazantsev wrote:
> 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.
In theory it's possible, and we can do smarter things in such cases. For example, if we have multiple select inputs with same Cond value, address still can be translated for Cond = true and Cond = false, and for that reason I'm passing Cond value and bool flag instead of select true or false value directly (like for translation of PHI nodes: we are passing CurBB->PredBB edge, not phi incoming value directly). However, on my experiments with SPEC2017, there was no cases with multiple select inputs, so maybe we could simplify this without any practical loss.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142705