This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] fix shuffle-of-binops transform to avoid poison/undef
ClosedPublic

Authored by spatel on Jul 6 2018, 3:20 PM.

Details

Summary

As noted in D48987, there are many different ways for this transform to go wrong. In particular, the poison potential for shifts means we have to more careful with those ops. I added tests to make that behavior visible for all of the different cases that I could find.

This is a partial fix. To make this review easier, I did not make changes for the single binop pattern (handled in foldSelectShuffleWith1Binop()). I also left out some potential optimizations noted with TODO comments. I'll follow-up once we're confident that things are correct here.

The goal is to correct all marked FIXME tests (assuming I identified those correctly) to either avoid the shuffle transform or do it safely.

Note that distinguishing when the shuffle mask contains undefs and using getBinOpIdentity() allows for some improvements to div/rem patterns, so there are wins along with the missed opportunities and fixes.

Diff Detail

Repository
rL LLVM

Event Timeline

spatel created this revision.Jul 6 2018, 3:20 PM

LGTM.

BTW, I couldn't figure if this case is possible, but mentioning it just in case: it's also not legal to introduce 'sdiv undef, ..', since 'sdiv INT_MIN, -1' is UB. Same for srem. So we need to be careful when folding the shuffle on LHS as well.

spatel added a comment.Jul 9 2018, 6:22 AM

LGTM.

BTW, I couldn't figure if this case is possible, but mentioning it just in case: it's also not legal to introduce 'sdiv undef, ..', since 'sdiv INT_MIN, -1' is UB. Same for srem. So we need to be careful when folding the shuffle on LHS as well.

Good point - it's not possible for now because we bail out on a LHS constant for div/rem for both patterns (1 variable operand or 2).

spatel accepted this revision.Jul 9 2018, 6:23 AM

(marking as accepted here in Phab to avoid unknown state with the commit)

This revision is now accepted and ready to land.Jul 9 2018, 6:23 AM
This revision was automatically updated to reflect the committed changes.