[PATCH] D125951: [InstCombine] bitcast (extractelement (bitcast x), index) -> X

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a subscriber: craig.topper.
spatel added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp:2370
+  Value *X;
+  if (match(VecOp, m_OneUse(m_BitCast(m_Value(X)))) && X->getType() == DestType)
+    return IC.replaceInstUsesWith(BitCast, X);
----------------
One-use should not be here - this transform replaces a value with an existing value, so it is always profitable.


================
Comment at: llvm/test/Transforms/InstCombine/bitcast-inseltpoison.ll:366
   %bc1 = bitcast <2 x i32> %A to <1 x i64>
   %ext = extractelement <1 x i64> %bc1, i32 0
   %bc2 = bitcast i64 %ext to <2 x i32>
----------------
This extractelement of a 1-element vector doesn't seem like the best canonicalization - it could just be a bitcast from vector to scalar. But we have this unusual inverse transform:
https://github.com/llvm/llvm-project/blob/cefe472c51fbcd1aed4d4a090709f25a12a8bc2c/llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp#L2750

That potentially creates 2 instructions (extract + bitcast) from a single bitcast. I'm not sure why we do that, but there is some potential regression with the x86 MMX type (see a bit above that in the source code for a similar transform). @craig.topper or @RKSimon might have some ideas on that.

Ideally, we would transform any <1 x Elt> vector extract instruction in the other direction (extelt --> bitcast). Then we would not need this patch because all of the bitcasts would collapse with existing folds. 

If that is not possible, then it would at least be better to have this patch moved over to InstSimplify - we are not creating any new instructions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125951