Page MenuHomePhabricator

[InstCombine] refine UB-handling in shuffle-binop transform
ClosedPublic

Authored by spatel on Jun 3 2018, 7:40 AM.

Details

Summary

As noted in rL333782, we can be both better for optimization and safer with this transform:
BinOp (shuffle V1, Mask), C --> shuffle (BinOp V1, NewC), Mask

The only potentially unsafe-to-speculate binops are integer div/rem. All other binops are always safe (although I don't see a way to assert that in code here).

For opcodes like shifts that can produce poison, it can't matter here because we know the lanes with undef are dropped by the subsequent shuffle.

Diff Detail

Repository
rL LLVM

Event Timeline

spatel created this revision.Jun 3 2018, 7:40 AM
lebedev.ri added inline comments.Jun 3 2018, 7:50 AM
lib/Transforms/InstCombine/InstructionCombining.cpp
1427–1428 ↗(On Diff #149638)

No special-handling for fdiv/frem?

1431 ↗(On Diff #149638)

We want int(1) even for floats?

test/Transforms/InstCombine/vec_shuffle.ll
855 ↗(On Diff #149638)

This looks like a float NAN?

spatel added inline comments.Jun 3 2018, 8:18 AM
lib/Transforms/InstCombine/InstructionCombining.cpp
1427–1428 ↗(On Diff #149638)

No - this was clarified recently following the llvm-dev discussion about FP undef:
D44216

1431 ↗(On Diff #149638)

We're only changing integer opcodes/operands here. I can assert that if it would make the code clearer?

test/Transforms/InstCombine/vec_shuffle.ll
855 ↗(On Diff #149638)

Yes. Interesting - I didn't notice that test was different than the rest of the FP cases!

So what happens here:

  1. This transform puts in an undef element in the constant vector.
  2. We canonicalize fsub with constant operand 1 to fadd: // X - C --> X + (-C)
IC: Visiting:   %1 = fsub <2 x float> %x, <float 4.200000e+01, float undef>
IC: Old =   %1 = fsub <2 x float> %x, <float 4.200000e+01, float undef>
    New =   <badref> = fadd <2 x float> %x, <float -4.200000e+01, float 0x7FF8000000000000>

...and in that constant conversion, we constant fold the undef vector element to a NaN constant.

This makes sense to me, but i'm not really familiar with shufflevector,
so i'm going to leave the actual LGTM to others.

lib/Transforms/InstCombine/InstructionCombining.cpp
1420 ↗(On Diff #149638)

Then i'd suggest

// With integer div/rem instructions, it is not safe to use a vector with undef

but that will require reflowing the entire comment :S

1431 ↗(On Diff #149638)

I think this is precisely the case of things to assert, so ideally yes please.

spatel updated this revision to Diff 149643.Jun 3 2018, 8:52 AM
spatel marked 2 inline comments as done.

Patch updated:
Make it clearer via code comment and assert that we are only dealing with integer opcodes/constants (FP binops are always safe to speculate).

spatel added a comment.Jun 3 2018, 9:10 AM

This makes sense to me, but i'm not really familiar with shufflevector,
so i'm going to leave the actual LGTM to others.

No problem - thanks for the suggestions! @efriedma noticed the potential holes in my initial fix, so I'll certainly wait for him to comment.

This revision is now accepted and ready to land.Jun 4 2018, 11:08 AM
This revision was automatically updated to reflect the committed changes.