constant folding fails on vicmp/vfcmp instructions with undef operands

[Quuxplusone]

Bugzilla Link	PR2529
Status	RESOLVED FIXED
Importance	P normal
Reported by	Stefanus Du Toit (sdt@rapidmind.com)
Reported on	2008-07-07 16:51:08 -0700
Last modified on	2008-07-10 10:51:18 -0700
Version	trunk
Hardware	PC All
CC	llvm-bugs@lists.llvm.org
Fixed by commit(s)
Attachments
Blocks
Blocked by
See also

For example, try passing the following through opt -instcombine:

define <4 x i32> @main(i32 %argc, i8** %argv) {
entry:
    %foo = vicmp slt <4 x i32> <i32 undef, i32 undef, i32 undef, i32 undef>, <i32
undef, i32 undef, i32 undef, i32 undef>
    ret <4 x i32> %foo
}

This generates the following assertion:
opt: Value.cpp:319: void llvm::Value::replaceAllUsesWith(llvm::Value*):
Assertion `New->getType() == getType() && "replaceAllUses of value with new
value of different type!"' failed.

With the following backtrace:
#4  0x08680fa3 in llvm::Value::replaceAllUsesWith (this=0x87d57c8,
New=0x87d4a00) at Value.cpp:318
#5  0x0843b35e in AddReachableCodeToWorklist (BB=0x87d6a78,
Visited=@0xbfaabfd8, IC=@0x87d5180, TD=0x87d50b0) at
InstructionCombining.cpp:11245

Which is the last line of:
      if (Constant *C = ConstantFoldInstruction(Inst, TD)) {
        DOUT << "IC: ConstFold to: " << *C << " from: " << *Inst;
        Inst->replaceAllUsesWith(C);

It appears the vicmp got folded into an i1 undef, instead of a <4 x i32> undef.

The same problem exists for vfcmp as well.

This was tested on r53196.

[Quuxplusone]

Fixed, thanks!
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20080707/064662.html

[Quuxplusone]

Thanks for the speedy fix. This fixed vicmp, but vfcmp still fails for me
(r53241):

define <4 x i32> @test(i32 %argc, i8** %argv) {
entry:
    %foo = vfcmp ueq <4 x float> <float undef, float undef, float undef, float
undef>, <float undef, float undef, float undef, float undef>
    ret <4 x i32> %foo
}

opt -instcombine crashes in the same spot.

[Quuxplusone]

Looking into this a little it appears the patch doesn't actually fix vector
comparisons with undef to return undef. In fact, in the vicmp slt case, the
instructions is folded into a vector of zeroes!

One problem is this:
  if (isa<UndefValue>(C1) || isa<UndefValue>(C2)) {

A vector of undefs is not an instance of UndefValue (apparently), so this
condition is false. In the case of a vicmp, this falls into the
"evaluateICmpRelation" path which apparently decides that the undef operands
are equal (should it really?).

In the case of vfcmp, it fails elsewhere.

First off, this piece of code is suspicious (ConstantFold.cpp:1353):

      if (pred == FCmpInst::FCMP_OEQ || pred == FCmpInst::FCMP_UEQ) {
        for (unsigned i = 0, e = CP1->getNumOperands(); i != e; ++i) {
          Constant *C = ConstantExpr::getFCmp(FCmpInst::FCMP_OEQ,
                                              CP1->getOperand(i),
                                              CP2->getOperand(i));
          if (ConstantInt *CB = dyn_cast<ConstantInt>(C))
            return CB;
        }

This is in an if statement that has already determined that C1/C2 are vectors.
This will return an i1 if two corresponding components in the vectors are
constants (not undefs!). Clearly that's just completely wrong.

I confirmed this by trying:
    %foo = vfcmp ueq <4 x float> <float 0.0, float 0.0, float 0.0, float undef>,
<float 1.0, float 1.0, float 1.0, float undef>

Which leads to the replaceAllUses assertion.

I also happened to notice that using entirely constant values crashes
instcombine somewhere else:

    %foo = vfcmp ueq <4 x float> <float 0.0, float 0.0, float 0.0, float 0.0>,
<float 1.0, float 1.0, float 1.0, float 1.0>

causes infinite recursion with this in the backtrace:

#3792 0x0869548a in llvm::ConstantFoldCompareInstruction (pred=32,
C1=0x87d4d58, C2=0x87d6c98) at ConstantFold.cpp:1445
#3793 0x08609a96 in llvm::ConstantExpr::getICmp (pred=32, LHS=0x87d4d58,
RHS=0x87d6c98) at Constants.cpp:2134
#3794 0x086938c4 in evaluateICmpRelation (V1=0x87d4d58, V2=0x87d6c98,
isSigned=false) at ConstantFold.cpp:1035

Returning back to the vfcmp undef case, this case doesn't return a constant
from ConstantFoldCompareInstruction, but instead continues on in getFCmp, where
it reaches this point:

  return ExprConstants->getOrCreate(Type::Int1Ty, Key);

which of course will create an i1. In fact, it returns this:

 i1 fcmp ueq (<4 x float> < float undef, float undef, float undef, float undef >, <4 x float> < float undef, float undef, float undef, float undef >)

Obviously that's not right :)

[Quuxplusone]

I've fixed the issues you identified.  If you hit more cases, please open a new
bug and include testcases, rather than reopening this one.  Thanks,

-Chris

http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20080707/064837.html
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20080707/064840.html
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20080707/064841.html

[Quuxplusone]

Thanks Chris! Will do.