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

Andrea Di Biagio via llvm-commits llvm-commits at lists.llvm.org
Mon Sep 28 10:25:16 PDT 2015


andreadb created this revision.
andreadb added reviewers: hfinkel, majnemer, spatel.
andreadb added a subscriber: llvm-commits.

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


http://reviews.llvm.org/D13219

Files:
  lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp
  test/Transforms/InstCombine/vec_demanded_elts.ll

Index: test/Transforms/InstCombine/vec_demanded_elts.ll
===================================================================
--- test/Transforms/InstCombine/vec_demanded_elts.ll
+++ test/Transforms/InstCombine/vec_demanded_elts.ll
@@ -253,3 +253,16 @@
   %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
+}
Index: lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp
===================================================================
--- lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp
+++ lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp
@@ -1057,7 +1057,10 @@
     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);


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D13219.35880.patch
Type: text/x-patch
Size: 1666 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150928/71d69d49/attachment.bin>


More information about the llvm-commits mailing list