[PATCH] D20774: [InstCombine] look through bitcasts to find selects

[Sanjay Patel via llvm-commits]

spatel added inline comments.

================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1335
@@ +1334,3 @@
+
+  // Canonicalize SExt, Not, or BitCast to the LHS.
+  if (match(Op1, m_SExt(m_Value())) || match(Op1, m_Not(m_Value())) ||
----------------
eli.friedman wrote:
> This canonicalization seems incomplete... for example, if both operands are sext instructions, the one which belongs on the LHS is the one where the source is a bool, but this just picks randomly.
Yes, you're right. I was going to say it is just an existing bug for the sext/not cases, but it seems we don't optimize the bitcast case either:

  define <2 x i64> @vecBitcastOp0(<4 x i32> %a, <8 x i16> %b) {
    %bc1 = bitcast <4 x i32> %a to <2 x i64>
    %bc2 = bitcast <8 x i16> %b to <2 x i64>
    %and = and <2 x i64> %bc1, %bc2
    ret <2 x i64> %and
  }

I thought we'd eliminate one bitcast (randomly?) for this case and do the logic op in the format of one of the inputs. Is that a valid IR optimization?


================
Comment at: lib/Transforms/InstCombine/InstCombineAndOrXor.cpp:1359
@@ +1358,3 @@
+  // so the types all line up. We're assuming that bitcasts are free and, as
+  // above, a select is cheaper than the combination of sext + and.
+  //
----------------
eli.friedman wrote:
> Assuming bitcasts are free is kind of a big assumption... you could easily end up in situations where the bitcasts are not free.  If X is scalar, the extra bitcasts should be folded away, but you could end up with bad effects on code like "((uint64_t)(vec1 == vec2)) & x".
Would it be fair to assume that vector bitcasts to another vector type are free? If not, then I suppose I need to abandon this patch and try again in the DAG?


http://reviews.llvm.org/D20774