This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by andreadb on Sep 28 2015, 10:25 AM.

Details

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

Diff Detail

Event Timeline

andreadb updated this revision to Diff 35880.Sep 28 2015, 10:25 AM
andreadb retitled this revision from to [InstCombine] SimplifyDemandedVectorElts wrongly analyzes ConstantVector select masks with ConstantExpr elements (PR24922)..
andreadb updated this object.
andreadb added reviewers: hfinkel, majnemer, spatel.
andreadb added a subscriber: llvm-commits.
spatel accepted this revision.Oct 5 2015, 10:17 AM
spatel edited edge metadata.

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.

This revision is now accepted and ready to land.Oct 5 2015, 10:17 AM
This revision was automatically updated to reflect the committed changes.