Page MenuHomePhabricator

[InstCombine] drop poison flags for shuffle transforms with undefs
AbandonedPublic

Authored by spatel on Jul 5 2018, 12:11 PM.

Details

Summary

As discussed in D48893, we have an existing bug in the recent shuffle transforms when we combine poison flags with undefs in the shuffle mask.

There are 2 ways to solve that bug: (1) change the undef constants to safe constants (which we must do for div/rem because they have immediate UB potential), or (2) drop the poison flags.

I suggested using safe constants in D48893, but I changed my mind for 2 reasons: (1) we can't always be sure what qualifies as a safe constant (operand 0 of a binop?) and (2) undef vector lanes are likely more valuable for further analysis and vector transforms than any poison decorations.

Diff Detail

Event Timeline

spatel created this revision.Jul 5 2018, 12:11 PM

So my concerns weren't unfounded :/
Do we want to also do such kind of fixup in VisitShl() in instcombine, for every shl we visit?

So my concerns weren't unfounded :/
Do we want to also do such kind of fixup in VisitShl() in instcombine, for every shl we visit?

Do you have an example of the problem you're thinking of? AFAIK, we won't hit this problem with scalars or general transforms because they don't modify constants like we're doing here. We do have a related foldShuffledBinop() function that I will review/fix if this patch looks right.

So my concerns weren't unfounded :/
Do we want to also do such kind of fixup in VisitShl() in instcombine, for every shl we visit?

Do you have an example of the problem you're thinking of? AFAIK, we won't hit this problem with scalars or general transforms because they don't modify constants like we're doing here. We do have a related foldShuffledBinop() function that I will review/fix if this patch looks right.

Note also that most of instcombine drops flags by default when it does a transform, and we generally favor dropping flags if that results in a better instruction canonicalization.

My main concern with these recent shuffle patches was not dropping fast-math-flags because that can inhibit later optimizations. I was too aggressive by trying to preserve poison flags along with those.

lebedev.ri added inline comments.Jul 5 2018, 1:02 PM
test/Transforms/InstCombine/shuffle_select.ll
30–42

So my concerns weren't unfounded :/
Do we want to also do such kind of fixup in VisitShl() in instcombine, for every shl we visit?

Do you have an example of the problem you're thinking of? AFAIK, we won't hit this problem with scalars or general transforms because they don't modify constants like we're doing here. We do have a related foldShuffledBinop() function that I will review/fix if this patch looks right.

https://godbolt.org/g/y7hCki

Should instcombine be dropping these flags just because?
Or we only care about not creating more of these situations than we already had?

spatel added inline comments.Jul 5 2018, 1:11 PM
test/Transforms/InstCombine/shuffle_select.ll
30–42

No, we don't want to drop flags on existing instructions. Presumably, whoever created that instruction did so correctly, so there actually is poison in the vector lane with undef.

We can't create poison where there wasn't any to start with, but we also don't want to remove poison when it exists without good reason. At least that's my understanding...happy to be corrected by people closer to poison/undef than me. :)

lebedev.ri accepted this revision.Jul 5 2018, 1:31 PM

LG, but maybe @nlopes wants to comment.

test/Transforms/InstCombine/shuffle_select.ll
30–42

Ok, makes sense :)

This revision is now accepted and ready to land.Jul 5 2018, 1:31 PM
nlopes added inline comments.Jul 5 2018, 2:31 PM
test/Transforms/InstCombine/shuffle_select.ll
521

Everything looks good, except this one.
As discussed in the other patch, a << undef is poison, so you can't introduce it. (since it's not correct to replace undef with poison).

spatel added inline comments.Jul 5 2018, 3:32 PM
test/Transforms/InstCombine/shuffle_select.ll
521

Hmm...how is this case different than the 'shl_shl' above?

Are there 4 cases?

  1. Instructions with immediate UB (srem, urem).
  2. Instructions with immediate UB and poison with flags (sdiv, udiv).
  3. Instructions with poison regardless of flags (shl, ashr, lshr).
  4. Instructions with poison only with flags (mul, add, sub).
nlopes added inline comments.Jul 6 2018, 1:12 AM
test/Transforms/InstCombine/shuffle_select.ll
521

This case is the same as shl_shl, yes.

Your 4 cases are correct. I would only add that case 3 is poison regardless of flags + poison with flags.

spatel abandoned this revision.Jul 6 2018, 3:36 PM

Abandoning - see D49047 instead.