This is an archive of the discontinued LLVM Phabricator instance.

Fix incorrect folding of an ordered fcmp with a vector of all NaN.
ClosedPublic

Authored by andreadb on Sep 1 2016, 10:14 AM.

Details

Summary

This patch fixes a crash caused by an incorrect folding of an ordered comparison between a packed floating point vector and a splat vector of NaN.

An ordered comparison between a vector and a constant vector of NaN, should always be folded into a constant vector where each element is i1 false.
Since revision 266175, SimplifyFCmpInst folds the ordered fcmp into a scalar i1 'false'. Later on, this would cause an assertion failure, since the value type of the folded value doesn't match the expected value type of the uses of the original instruction:
"Assertion failed: New->getType() == getType() && "replaceAllUses of value with new value of different type!".

This patch fixes the issue and adds a test case to the already existing test InstSimplify/floating-point-compares.ll.

Please let me know if okay to commit.

Thanks,
Andrea

Diff Detail

Repository
rL LLVM

Event Timeline

andreadb updated this revision to Diff 70024.Sep 1 2016, 10:14 AM
andreadb retitled this revision from to Fix incorrect folding of an ordered fcmp with a vector of all NaN..
andreadb updated this object.
andreadb added reviewers: majnemer, spatel.
andreadb added a subscriber: llvm-commits.
spatel accepted this revision.Sep 2 2016, 7:25 AM
spatel edited edge metadata.

LGTM.

Given the number of times we duplicate this line in this function, we could make this parallel the code in SimplifyICmpInst:

Type *RetTy = GetCompareTy(LHS); // at the top of the function
getFalse(RetTy); // everywhere below
This revision is now accepted and ready to land.Sep 2 2016, 7:25 AM

LGTM.

Given the number of times we duplicate this line in this function, we could make this parallel the code in SimplifyICmpInst:

Type *RetTy = GetCompareTy(LHS); // at the top of the function
getFalse(RetTy); // everywhere below

Thanks Sanjay!

I agree that we can avoid the code duplication (even for the case where 'true' is returned).
I commit this for now and then do the cleanup on a separate NFC patch.

Cheers,
Andrea

This revision was automatically updated to reflect the committed changes.