[PATCH] D52548: Stop instcombining introducing undef's in div/rem instructions.
Sanjay Patel via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Thu Sep 27 10:12:01 PDT 2018
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
More information about the llvm-commits
mailing list