llvm dev - Jan 2015 - [LLVMdev] 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.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: bug1.ll
Type: application/octet-stream
Size: 203 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150114/83ad20d1/attachment.obj>

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<ConstantFP>(V))
-    return ConstantExpr::getFNeg(C);
-  if (Constant *C = dyn_cast<Constant>(V))
+  if (Constant *C = dyn_cast<Constant>(V)) {
+    if (C->getType()->isFPOrFPVectorTy()) {
+      return ConstantExpr::getFNeg(C);
+    }
     return ConstantExpr::getNeg(C);
+  }

On Wed, Jan 14, 2015 at 9:42 PM, Mehdi Amini <mehdi.amini at apple.com>
wrote:
> 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.
>
>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150114/cf92de4a/attachment.html>

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
> On Jan 14, 2015, at 10:31 PM, David Majnemer <david.majnemer at
gmail.com> wrote:
> 
> 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<ConstantFP>(V))
> -    return ConstantExpr::getFNeg(C);
> -  if (Constant *C = dyn_cast<Constant>(V))
> +  if (Constant *C = dyn_cast<Constant>(V)) {
> +    if (C->getType()->isFPOrFPVectorTy()) {
> +      return ConstantExpr::getFNeg(C);
> +    }
>      return ConstantExpr::getNeg(C);
> +  }
> 
> On Wed, Jan 14, 2015 at 9:42 PM, Mehdi Amini <mehdi.amini at apple.com
<mailto:mehdi.amini at apple.com>> wrote:
> 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.
> 
> 
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu <mailto:LLVMdev at cs.uiuc.edu>        
http://llvm.cs.uiuc.edu <http://llvm.cs.uiuc.edu/>
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
<http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev>
> 
> 
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20150115/d4b82b5a/attachment.html>