[PATCH] D48678: [InstCombine] enhance shuffle-of-binops to allow different variable ops (PR37806)

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri requested changes to this revision.
lebedev.ri added inline comments.
This revision now requires changes to proceed.


================
Comment at: test/Transforms/InstCombine/shuffle_select.ll:411-423
+; Div/rem need special handling if the shuffle has undef elements.
+
 define <4 x i32> @srem_2_vars(<4 x i32> %v0, <4 x i32> %v1) {
 ; CHECK-LABEL: @srem_2_vars(
-; CHECK-NEXT:    [[T1:%.*]] = srem <4 x i32> <i32 1, i32 2, i32 3, i32 4>, [[V0:%.*]]
-; CHECK-NEXT:    [[T2:%.*]] = srem <4 x i32> <i32 5, i32 6, i32 7, i32 8>, [[V1:%.*]]
-; CHECK-NEXT:    [[T3:%.*]] = shufflevector <4 x i32> [[T1]], <4 x i32> [[T2]], <4 x i32> <i32 0, i32 undef, i32 6, i32 3>
+; CHECK-NEXT:    [[TMP1:%.*]] = shufflevector <4 x i32> [[V0:%.*]], <4 x i32> [[V1:%.*]], <4 x i32> <i32 0, i32 undef, i32 6, i32 3>
+; CHECK-NEXT:    [[T3:%.*]] = srem <4 x i32> <i32 1, i32 1, i32 7, i32 4>, [[TMP1]]
 ; CHECK-NEXT:    ret <4 x i32> [[T3]]
----------------
spatel wrote:
> lebedev.ri wrote:
> > `%TMP1`'s second element is `undef`, since the mask is `undef`, no?
> > So we are `srem`'ing by the vector with one known `undef`.
> > That's ok?
> Ah, good point. I don't think we currently have the analysis to cause trouble there, but we might in the future. 
> 
> So we need to replace the undef in the shuffle mask with the proper lane that selects from one of the source vectors. That might be forbidden because we'd be creating a more specific shuffle mask than already existed. Should we just give up on trying to fold div/rem in this case?
> Should we just give up on trying to fold div/rem in this case?

I would think so, at least for now.



https://reviews.llvm.org/D48678