Handling of undef in the IR

Hi all,

I have a very simple test case (thanks to bugpoint) that hit an assert in reassociate.
(the assert is (C->getType()->isIntOrIntVectorTy() && "Cannot NEG a nonintegral value!"), function getNeg)

The function is taking a Constant as argument, but the assert does not expect an undef. I’m not sure whose responsibility is it to handle that (caller?).
Do we have to defensively assume that any Constant can be an undef anywhere and I should just add a check here?

Thanks,

Mehdi

PS: IR attached for reference.

bug1.ll (203 Bytes)

Hi Mehdi,

I think this is a simple bug in Reassociate.

It’s assuming that objects which are Constant but not ConstantFP must have a type which isn’t floating point.

The code in question should be dispatching off of the type, not the particular subclass of the Constant hierarchy.

The following patch seems to work fine and passes all the tests in the repository:

diff --git a/lib/Transforms/Scalar/Reassociate.cpp b/lib/Transforms/Scalar/Reassociate.cpp
index 4e02255…f618268 100644
— a/lib/Transforms/Scalar/Reassociate.cpp
+++ b/lib/Transforms/Scalar/Reassociate.cpp
@@ -917,10 +917,12 @@ void Reassociate::RewriteExprTree(BinaryOperator *I,
/// version of the value is returned, and BI is left pointing at the instruction
/// that should be processed next by the reassociation pass.
static Value *NegateValue(Value *V, Instruction *BI) {

  • if (ConstantFP *C = dyn_cast(V))
  • return ConstantExpr::getFNeg(C);
  • if (Constant *C = dyn_cast(V))
  • if (Constant *C = dyn_cast(V)) {
  • if (C->getType()->isFPOrFPVectorTy()) {
  • return ConstantExpr::getFNeg(C);
  • }
    return ConstantExpr::getNeg(C);
  • }

Hi David

Your change will actually be a change in behavior as 'C->getType()->isFPOrFPVectorTy()’ will catch float vectors in ConstantAggregateZero. Also looks like floats can be in ConstantDataVector or maybe even ConstantDataArray although I don’t see an easy way to ensure thats what we parse from an .ll file.

So I think its a good change, but we should probably add a test case for this new behavior, if its even possible to get a 0.0 all the way through to this point, which it might not be.

Thanks,
Pete

Actually please ignore me. After talking with Mehdi its clear that all the subclasses of Constant I mentioned would have triggered the same assert Mehdi originally found.

Sorry for the noise.

Thanks,
Pete