Page MenuHomePhabricator

[InstSimplify][InstCombine] don't crash when folding vector selects of icmp
ClosedPublic

Authored by spatel on Jul 20 2016, 3:38 PM.

Details

Summary

As Eli pointed out in D22537 , we have big chunks of duplicated code for similar folds in InstSimplify and InstCombine. I started on the path to refactoring that in rL276140, but it looks like it's going to be a long haul to make sure vectors work in all cases.

As Eli also noted in D22537, the code doesn't work for vector types. Not only does it not fold, it crashes. So this is hopefully a quick fix for both places before I get back to the refactoring job.

I think we need to keep the 'getTypeSizeInBits()' call to handle aggregate types, but use getScalarSizeInBits() for vectors.

Note that I've added a scalar test for the InstCombine fold because it doesn't exist anywhere. There are likely many more of these patterns that simply have no tests. I plan to continue adding those in subsequent patches.

Diff Detail

Repository
rL LLVM

Event Timeline

spatel updated this revision to Diff 64779.Jul 20 2016, 3:38 PM
spatel retitled this revision from to [InstSimplify][InstCombine] don't crash when folding vector selects of icmp.
spatel updated this object.
spatel added a subscriber: llvm-commits.
spatel updated this revision to Diff 64782.Jul 20 2016, 3:46 PM
spatel updated this object.

Patch updated: we can use getScalarType() to make this cleaner.

ibadawi accepted this revision.Jul 20 2016, 4:08 PM
ibadawi edited edge metadata.

Makes sense to me. I also applied your patch and verified that I was able to compile the original file that I reduced to get my reproducer.

Thanks for the quick turnaround on this.

This revision is now accepted and ready to land.Jul 20 2016, 4:08 PM
majnemer accepted this revision.Jul 20 2016, 4:13 PM
majnemer edited edge metadata.

LGTM

For the record, it doesn't make any sense to get a sign bit for an aggregate, but I don't think we can ever reach a bug from that because we can't icmp an aggregate. I'll clean this up when it's rewritten.

This revision was automatically updated to reflect the committed changes.