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

[Eli Friedman via llvm-commits]

eli.friedman 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())) ||
----------------
spatel wrote:
> 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?
> 
Well, you can't transform the return type of a function unconditionally; it could change the calling convention.

But to actually answer your question, you can perform an "and" in any type of the right size as long as there aren't any padding bits involved; "and" is a bit-wise operation. Whether that's a good idea probably depends on the target; performing an "and" in an illegal type is likely to be expensive.

================
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.
+  //
----------------
spatel wrote:
> 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?
Vector to vector bitcasts are free on most platforms, but not all.  As a practical example, big-endian AArch64 has non-trivial vector bitcasts: a bitcast translates to a vector shuffle to put the elements in the right places.

In theory, there's infrastructure for querying costs like this from transform passes; see TargetTransformInfo::getCastInstrCost.  Not sure how that would work out in practice.


http://reviews.llvm.org/D20774