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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Analysis/InstructionSimplify.cpp:4044
+        NewC.push_back(TEltC);
+      }
+    }
----------------
nikic wrote:
> 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.
Good point. We have to walk through the vector elements anyway, so we can save time and space by doing it all in one place. 


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

https://reviews.llvm.org/D72958