[PATCH] D138185: [InstCombine] Look through bitcast if possible

[Jay Foad via Phabricator via llvm-commits]

foad added a subscriber: spatel.
foad added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp:369
     if (Instruction *I = dyn_cast<Instruction>(U.getUser())) {
-      UnionUsedElts |= findDemandedEltsBySingleUser(V, I);
+      // Bitcast between vectors with the same element count does not change
+      // the demanded elements, so we are safe to look through them.
----------------
It might be neater to handle BitCast in findDemandedEltsBySingleUser instead. In either case, you're introducing recursion, so perhaps it should have a depth limit.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp:571
       // property.
-      if (SrcVec->hasOneUse()) {
+      auto *SimplifyVec = SrcVec;
+      Value *BitCastSrc = nullptr;
----------------
It took me a long time to understand this part of the patch. It is handling cases like this:
```
  %var = call ... // has multiple uses
  %var1 = bitcast <4 x i32> %var to <4 x float> // has single use
  %var2 = extractelement <4 x float> %var1, i64 0
```
If the operand of extractelement itself had multiple uses, then we would handle it correctly here by calling findDemandedEltsByAllUsers. But because it is wrapped inside a single-use bitcast, we call SimplifyDemandedVectorElts directly, which recurses through the bitcast but does //not// handle multiple uses at the recursive step.

Really all this code feels like a bit of a hack that we only call SimplifyDemandedVectorElts in certain places (when we think there's a good chance that not all elements are demanded) and we only handle multiple uses in certain places.

@spatel do you have any thoughts about this?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138185