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

[Max Kazantsev via Phabricator via llvm-commits]

mkazantsev added inline comments.


================
Comment at: llvm/include/llvm/Analysis/PHITransAddr.h:89
+        if (auto *SI = dyn_cast<SelectInst>(I))
+          return SI;
+    return nullptr;
----------------
kachkov98 wrote:
> 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.
With basic block check is now gone, my question still holds. What if there are many selects among `InstInputs`? This method's comment doesn't say anything about which condition it should return. Is there a guarantee that it'll be the first one, or it could theoretically return any of them? May this change or not? Is it important for the algorithm?

>From how you're using it, it seems that what you *really* want is to return all conditions of all involved selects (in vector or set, I don't know if order matters). I'm completely fine if it's not done in this patch, but the specification of this method isn't clear about this situation, and I'm not sure this method is doing what intended.


================
Comment at: llvm/include/llvm/Analysis/PHITransAddr.h:47
+    // Otherwise V1 must be SelectInst; return both addresses from its operands.
+    assert(isa<SelectInst>(V1));
+    auto *SI = cast<SelectInst>(V1);
----------------
`cast` already contains this assert.


================
Comment at: llvm/include/llvm/Analysis/PHITransAddr.h:49
+    auto *SI = cast<SelectInst>(V1);
+    return {SI->getTrueValue(), SI->getFalseValue()};
+  }
----------------
Can we do it in constructor? Smth like
```
  SelectAddr(Value *Addr) {
    if (auto *SI = dyn_cast<SelectInst>(Addr)) {
      V1 = SI->getTrueValue();
      V2 = SI->getFalseValue();
    } else {
      V1 = Addr;
      V2 = nullptr;
    }
  }
```
and just return `{V1, V2}` here? I don't know if it's possible to make many queries here, but if yes, it'll definitely be faster.

In that case, you can also replace
```
  Value *V1, *V2;
```
with
```
SelectAddrs  V;
```
and just return it.


================
Comment at: llvm/lib/Analysis/MemoryDependenceAnalysis.cpp:1041
+        return NonLocalDepResult(PredBB, MemDepResult::getDef(SI),
+                                 SelectAddr(TrueAddr, FalseAddr));
+  }
----------------
I don't understand what this code is doing. Imagine you meet smth like
```
  select i1 %cond, i32 undef, i32 undef
```
which is also dead. The search will stop and return this instruction (?). And if it doesn't exist, it will maybe return nullopt. What was the intention here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142705