[InstCombine] SimplifyDemandedVectorElts wrongly analyzes ConstantVector select masks with ConstantExpr elements (PR24922).

[andreadb]

Summary

This patch fixes PR24922.

If the mask of a select instruction is a ConstantVector, method SimplifyDemandedVectorElts iterates over its elements to identify which are selected from the first value argument and which are selected from the second value argument.

The problem with this logic is that method Constant::isNullValue() is used to check if a Constant value in the mask is zero. Unfortunately, that method always returns false if invoked on a ConstantExpr.

Before this patch, function @PR24922 in vec_demanded_elts.ll was wrongly folded to:
{code}

ret <2 x i64> %v

{code}

This patch fixes the problem in SimplifyDemandedVectorElts by adding an explicit check for ConstantExpr values. Now, if a value in the mask is a ConstantExpr, we avoid propagating wrong facts.

There are however two fundamental questions here:

1. Why InstructionSimplify didn't fold that constant expression?
2. Should ConstantExpr be simplified before we call SimplifyDemandedVectorElts?

It turns out that instsimplify cannot always simplify complex constant expressions. This was also observed in PR24965.
So, at least for now, we cannot promise that a ConstantVector doesn't have ConstantExpr elements in it. Therefore, relying on isNullValue() in this context is actually wrong (at least, it would propagate wrong results in the presence of ConstantExpr).

Please let me know what you think.

Thanks,
Andrea

[andreadb]

ping.

[spatel]

I don't know the answers to the general ConstantExpr questions, but this patch is a strict improvement / correctness fix, so LGTM.
There's a similar check here:
http://llvm.org/docs/doxygen/html/InstructionCombining_8cpp_source.html#l00838

I would add a similar comment in this patch to explain the reason we must distinguish ConstantExpr from Constant.

lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp

if (NewUndefElts) {
       MadeChange = true;
     }
     break;
   }
   case Instruction::Select: {
     APInt LeftDemanded(DemandedElts), RightDemanded(DemandedElts);
     if (ConstantVector* CV = dyn_cast<ConstantVector>(I->getOperand(0))) {
       for (unsigned i = 0; i < VWidth; i++) {
-        if (CV->getAggregateElement(i)->isNullValue())
+        Constant *CElt = CV->getAggregateElement(i);
+        if (isa<ConstantExpr>(CElt))
+          continue;
+        if (CElt->isNullValue())
           LeftDemanded.clearBit(i);
         else
           RightDemanded.clearBit(i);
       }
     }
 
     TmpV = SimplifyDemandedVectorElts(I->getOperand(1), LeftDemanded, UndefElts,
                                       Depth + 1);

test/Transforms/InstCombine/vec_demanded_elts.ll

 }
 
 define <4 x double> @test_vpermilvar_pd_256_zero(<4 x double> %v) {
 ; CHECK-LABEL: @test_vpermilvar_pd_256_zero(
 ; CHECK: shufflevector <4 x double> %v, <4 x double> undef, <4 x i32> <i32 0, i32 0, i32 2, i32 2>
   %a = tail call <4 x double> @llvm.x86.avx.vpermilvar.pd.256(<4 x double> %v, <4 x i64> zeroinitializer)
   ret <4 x double> %a
 }
+
+define <2 x i64> @PR24922(<2 x i64> %v) {
+; CHECK-LABEL: @PR24922
+; CHECK: select <2 x i1> 
+;
+; Check that instcombine doesn't wrongly fold the select statement into a
+; ret <2 x i64> %v
+;
+; FIXME: We should be able to simplify the ConstantExpr in the select mask.
+entry:
+  %result = select <2 x i1> <i1 icmp eq (i64 extractelement (<2 x i64> bitcast (<4 x i32> <i32 15, i32 15, i32 15, i32 15> to <2 x i64>), i64 0), i64 0), i1 true>, <2 x i64> %v, <2 x i64> zeroinitializer
+  ret <2 x i64> %result
+}