[PATCH] D72958: [InstSimplify] fold select of vector constants that include undef elements

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:4044
+        NewC.push_back(TEltC);
+      }
+    }
----------------
spatel wrote:
> nikic wrote:
> > I'm having a bit of a hard time convincing myself that this is always correct. In particular, the relationship between isElementWiseEqual(), which is based on `ConstantExpr::getICmp()` and this code, which is based on simple identity comparison, is not entirely clear. If the vector includes complex constant expressions, and `ConstantExpr::getICmp()` manages to fold that code while the identity comparison fails, this would trigger an assertion failure here. Or is there an invariant that this cannot happen?
> I've tried to concoct an example of this:
> 
> ```
> @g = global i32 0
> 
> define <3 x i32> @sel_constant_expression(<3 x i1> %cond) {
>   %r = select <3 x i1> %cond, <3 x i32> <i32 ptrtoint (i32* @g to i32), i32 0, i32 1>, <3 x i32> <i32 bitcast (<2 x i16> bitcast (i32 ptrtoint (i32* @g to i32) to <2 x i16>) to i32), i32 0, i32 undef>
>   ret <3 x i32> %r
> }
> ```
> 
> ...but the 2nd ConstantExpr get constant folded into the reduced form of the 1st (the bitcasts go away). So we can either leave this as-is, or we could bail out on ConstantExpr to be safe(r)? I can't imagine that this will ever make a difference in any real-world code.
I wasn't able to come up with an example either.

I do wonder though if this code shouldn't just forgo `isElementWiseEqual()` entirely and just cover all four cases in this if/else chain instead? I.e. equality, undef op1, undef op2, else abort the transform. Going through `isElementWiseEqual()` doesn't really seem to make this code simpler.


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

https://reviews.llvm.org/D72958