[PATCH] D52548: Stop instcombining introducing undef's in div/rem instructions.

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

In https://reviews.llvm.org/D52548#1248149, @spatel wrote:

> I haven't actually stepped through the code changes yet, so let me know if I'm missing something. I'm just looking at the tests a bit closer now.
>
> The problem is independent of any FP ops, so we can kill the int-to-fp-cast ops and the trailing fmul in all cases, and we still show the bug.
>  Going further, we don't even need the final shuffle in any of these tests?
>
>   define <3 x i32> @add(i32 %y, i32 %z) {
>     %i0 = insertelement <2 x i32> undef, i32 %y, i32 0
>     %i1 = insertelement <2 x i32> %i0, i32 %z, i32 1
>     %a = udiv <2 x i32> %i1, <i32 255, i32 255>
>     %ext = shufflevector <2 x i32> %a, <2 x i32> undef, <3 x i32> <i32 0, i32 1, i32 undef>
>     ret <3 x i32> %ext
>   }
>  
>


And I gave away the punchline in that test name. :)
This is where I was heading, and I've now checked. Let's change the div to a seemingly safe 'add' instead:

  define <3 x i32> @add(i32 %y, i32 %z) {
    %i0 = insertelement <2 x i32> undef, i32 %y, i32 0
    %i1 = insertelement <2 x i32> %i0, i32 %z, i32 1
    %a = add <2 x i32> %i1, <i32 255, i32 255>
    %ext = shufflevector <2 x i32> %a, <2 x i32> undef, <3 x i32> <i32 0, i32 1, i32 undef>
    ret <3 x i32> %ext
  }

  $ ./opt -instcombine badshuf.ll -S
  define <3 x i32> @add(<3 x i32> %x, i32 %y, i32 %z) {
    %1 = insertelement <3 x i32> undef, i32 %y, i32 0
    %2 = insertelement <3 x i32> %1, i32 %z, i32 1
    %3 = add <3 x i32> %2, <i32 255, i32 255, i32 undef>
    ret <3 x i32> %3
  }

That's probably not a desired canonicalization for any op. Yes, we eliminated a shuffle, but we created wider vector ops to make that happen, and that's not something we should do without a cost model.
So I think there's a simpler fix for the problem as demonstrated - don't call CanEvaluateShuffled() if this is shuffle that widens its inputs. There might still be a need for preventing undefs with div/rem, but we need a different test to show that.


https://reviews.llvm.org/D52548