This is an archive of the discontinued LLVM Phabricator instance.

[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

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
1441–1442

No special-handling for fdiv/frem?

1445

We want int(1) even for floats?

test/Transforms/InstCombine/vec_shuffle.ll
855

This looks like a float NAN?

spatel added inline comments.Jun 3 2018, 8:18 AM
lib/Transforms/InstCombine/InstructionCombining.cpp
1441–1442

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

1445

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

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
1434–1435

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

1445

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.