HomePhabricator

[InstCombine] fix vector shuffle transform to replace undef elements (PR37648)

Description

[InstCombine] fix vector shuffle transform to replace undef elements (PR37648)

This bug:
https://bugs.llvm.org/show_bug.cgi?id=37648
...was created with the enhancement to this transform with rL332479.

The urem test shows the disaster potential: any undef divisor lane makes
the whole op undef.

The test diffs show that vector demanded elements turns some of the potential,
but not all, unused binop operands back into undef already.

Details

Committed
spatelJun 1 2018, 12:23 PM
Parents
rL333781: [lldb, process] Fix occasional hang when launching a process in LLDB
Branches
Unknown
Tags
Unknown

Event Timeline

efriedma added inline comments.
/llvm/trunk/lib/Transforms/InstCombine/InstructionCombining.cpp
1427

This isn't sufficient; given sdiv <4 x i32> %x, <i32 -1, i32 -2, i32 -4, i32 -8>, you can't pick -1.

spatel added inline comments.Jun 1 2018, 2:40 PM
/llvm/trunk/lib/Transforms/InstCombine/InstructionCombining.cpp
1427

But that won't make it past the earlier check for isSafeToSpeculativelyExecute(). Am I missing some other path down here for potentially dangerous ops?

efriedma added inline comments.Jun 1 2018, 3:05 PM
/llvm/trunk/lib/Transforms/InstCombine/InstructionCombining.cpp
1427

Oh, you're right, it won't make it past at the moment. But that's only because llvm::isSafeToSpeculativelyExecute is overly conservative at the moment; it could be improved to, for example, check whether the numerator is non-negative.

spatel added inline comments.Jun 1 2018, 3:42 PM
/llvm/trunk/lib/Transforms/InstCombine/InstructionCombining.cpp
1427

Good point. Also, I just noticed why the urem case wasn't already simplified correctly - we call SimplifyVectorOp before SimplifyURemInst. I'll try to clean this up and add more tests/asserts to ensure we don't break.