[DAGCombiner] Remove reduceBuildVecConvertToConvertBuildVec and rely on the vectorizers instead

[RKSimon]

Bugzilla Link	35732
Resolution	FIXED
Resolved on	May 24, 2021 06:47
Version	trunk
OS	Windows NT
Blocks	#48425
CC	@alexey-bataev,@anton-afanasyev,@efriedma-quic,@hfinkel,@darkbuck,@rotateright
Fixed by commit(s)	345964, ab2c499

Extended Description

reduceBuildVecConvertToConvertBuildVec vectorizes int2float in the DAGCombiner, which means that even if the LV/SLP has decided to keep scalar code, this will override this.

While there are cases where vectorization is necessary in the DAG (mainly due to legalization artefacts), I don't think this is the case here. I think we should get rid of this and instead assume that the vectorizers know what they are doing.

SVN blame says this code was added back in 2012.

[RKSimon]

assigned to @anton-afanasyev

[hfinkel]

reduceBuildVecConvertToConvertBuildVec vectorizes int2float in the
DAGCombiner, which means that even if the LV/SLP has decided to keep scalar
code, this will override this.

While there are cases where vectorization is necessary in the DAG (mainly
due to legalization artefacts), I don't think this is the case here. I think
we should get rid of this and instead assume that the vectorizers know what
they are doing.

SVN blame says this code was added back in 2012.

Originally, as I recall, the rationale for keeping this in DAGCombine was that, for short sequences of instructions, you needed to make the decision later in the pipeline (i.e. the SLP vectorizer won't vectorize short sequences, because the cost modeling won't be good enough at the IR level, but it will be better during isel). It would be nice, IMHO, if this were an incorrect assertion at this point, but that was the rationale.

[alexey-bataev]

I think this function is very similar to InstCombiner rather than the SLP Vectorizer. It tries to reorganize the 2*n of vector insert( scalar cast) instructions into n+1 (vector cast(vector insert)) instructions. I don't think it has something to do with the cost model.

[RKSimon]

I think this function is very similar to InstCombiner rather than the SLP
Vectorizer. It tries to reorganize the 2*n of vector insert( scalar cast)
instructions into n+1 (vector cast(vector insert)) instructions. I don't
think it has something to do with the cost model.

Sanjay - do you know if InstCombine has any equivalent? As I said, I'd like to remove this and rely on the vectorizer / cost models to know when this is worth it.

[rotateright]

I think this function is very similar to InstCombiner rather than the SLP
Vectorizer. It tries to reorganize the 2*n of vector insert( scalar cast)
instructions into n+1 (vector cast(vector insert)) instructions. I don't
think it has something to do with the cost model.

Sanjay - do you know if InstCombine has any equivalent? As I said, I'd like
to remove this and rely on the vectorizer / cost models to know when this is
worth it.

Let me make sure I'm understanding the pattern. Here's a reduction of the test added with:
https://reviews.llvm.org/rL166546

define <2 x float> @​foo(i32 %x, i32 %y) {
%xf = uitofp i32 %x to float
%yf = uitofp i32 %y to float
%v0 = insertelement <2 x float> undef, float %xf, i32 0
%v1 = insertelement <2 x float> %v0, float %yf, i32 1
ret <2 x float> %v1
}

---

reduceBuildVecConvertToConvertBuildVec() will effectively transform this into:

define <2 x float> @​foo(i32 %x, i32 %y) {
%v0 = insertelement <2 x i32> undef, float %x, i32 0
%v1 = insertelement <2 x i32> %v0, float %y, i32 1
%vf = uitofp <2 x i32> %v1 to <2 x float>
ret <2 x float> %vf
}

---

AFAIK, instcombine does not have this kind of transform even though it reduces the instruction count. It does not create vector cast/math ops out of scalar ops because it has no cost information to know if the vector op is supported/better.

[rotateright]

To make that clearer (and acknowledge what I think is suggested in comment 2): theoretically, we could/should canonicalize IR to the vector op because that's shorter IR.

But in practice, we're too scared to do that because the backend may not undo that transform if it's a perf loser. This is similar reasoning to why we don't aggressively form arbitrary shuffle ops in IR.

If the backend could guarantee splitting back to scalar when profitable, then we would create vector ops in instcombine.

We'd also have to make sure that instcombine's transforms didn't interfere with the vectorizers. I'm looking at an example of that as part of bug 34724 - if I adjust instcombine's shufflevector creation slightly, SLP no longer recognizes an opportunity to create vector math because it doesn't analyze the shuffle in the same way as an equivalent series of insertelement ops.

[alexey-bataev]

To make that clearer (and acknowledge what I think is suggested in comment
2): theoretically, we could/should canonicalize IR to the vector op because
that's shorter IR.

But in practice, we're too scared to do that because the backend may not
undo that transform if it's a perf loser. This is similar reasoning to why
we don't aggressively form arbitrary shuffle ops in IR.

If the backend could guarantee splitting back to scalar when profitable,
then we would create vector ops in instcombine.

We'd also have to make sure that instcombine's transforms didn't interfere
with the vectorizers. I'm looking at an example of that as part of bug 34724

• if I adjust instcombine's shufflevector creation slightly, SLP no longer
  recognizes an opportunity to create vector math because it doesn't analyze
  the shuffle in the same way as an equivalent series of insertelement ops.

Hmm, maybe then we should try to do this in SLP vectorizer.

[rotateright]

Hmm, maybe then we should try to do this in SLP vectorizer.

Yes, I don't see instcombine getting into even basic vectorization anytime soon.

For SLP, we probably need to start with a basic example that (hopefully) does not have cost model concerns:

define <4 x float> @​sitofp_4_scalars_to_1_vec(i32 %x, i32 %y, i32 %z, i32 %w) {
%xf = sitofp i32 %x to float
%yf = sitofp i32 %y to float
%zf = sitofp i32 %z to float
%wf = sitofp i32 %w to float
%v0 = insertelement <4 x float> undef, float %xf, i32 0
%v1 = insertelement <4 x float> %v0, float %yf, i32 1
%v2 = insertelement <4 x float> %v1, float %zf, i32 2
%v3 = insertelement <4 x float> %v2, float %wf, i32 3
ret <4 x float> %v3
}

The backend will currently turn that into 1 vector cast op:
vmovd %edi, %xmm0
vpinsrd $1, %esi, %xmm0, %xmm0
vpinsrd $2, %edx, %xmm0, %xmm0
vpinsrd $3, %ecx, %xmm0, %xmm0
vcvtdq2ps %xmm0, %xmm0

But I think we should allow SLP to do the same insertelement ops in IR.

[RKSimon]

Candidate Patch: https://reviews.llvm.org/D53712

[RKSimon]

DAGCombiner support was removed at rL345964.

The next step will be to improve SLPVectorizer support for cases like [Comment #​7].

[anton-afanasyev]

Assigned to myself. This patch fixes the last part of this issue: https://reviews.llvm.org/D98714

[anton-afanasyev]

The last part of issue is fixed by https://reviews.llvm.org/D98714.
Marking as resolved therefore.

[GabrielRavier]

mentioned in issue #48425