[X86] Drop single use check for freeze(undef) in LowerAVXCONCAT_VECTORS

[ManuelJBrito]

Summary

Ignoring freeze(undef) if it has multiple uses in LowerAVXCONCAT_VECTORS causes the custom INSERT_SUBVECTOR for vector widening to be ignored.

For example in https://godbolt.org/z/7hacPe1KM extra vinsertf128 instructions are introduced.

This is necessary to lower mm512_cast*128 intel intrinsics as no-op intrinsics.

[RKSimon]

clang/test/CodeGen/X86/avx-cast-builtins.c

1

Why did you lose this?

[ManuelJBrito]

Recover mistakenly drop flag in avx-cast-builtins.
I was experimenting and forgot i had removed it.

[RKSimon]

LGTM

[RKSimon]

@ManuelJBrito Any luck with getting this committed? Your first attempt was reverted but was it just because of the bad Differential Revision tag?

[ManuelJBrito]

I originally reverted it because of the wrong tag, but there were also some buildbot failures (see https://lab.llvm.org/buildbot/#/builders/139/builds/36736).
It appears to be failing an assert in DiagnosticsEngine::DiagStateMap::append, but i'm not very familiar with this part so i will have to investigate further.

[ManuelJBrito]

It seems the build failure was caused by a known crash https://github.com/llvm/llvm-project/issues/55263. I tried to find some workaround but unsuccessfully.

So I'm thinking I can drop the end-to-end tests for now and commit them when the crash is fixed and for now just rely on the separate frontend and backend tests for regressions.

Is this OK with you @RKSimon ?

[RKSimon]

In D144903#4169623, @ManuelJBrito wrote:

It seems the build failure was caused by a known crash https://github.com/llvm/llvm-project/issues/55263. I tried to find some workaround but unsuccessfully.

@pengfei Do you know what's going on with this bug?

So I'm thinking I can drop the end-to-end tests for now and commit them when the crash is fixed and for now just rely on the separate frontend and backend tests for regressions.

Is this OK with you @RKSimon ?

Let's see if we can unstick PR55263 first - yak shaving :(

[aqjune]

clang/test/CodeGen/X86/avx-cast-builtins.c

1

This line contains %s twice, which seems to cause the crash.

Would removing one of %s resolve the crash issue? On my machine removing one of those worked well; with two %s; it crashed.

1

Actually, with FileCheck's argument counted there are three %s, but it isn't important; I meant two %s in clang_cc1's arguments.

[ManuelJBrito]

clang/test/CodeGen/X86/avx-cast-builtins.c

1

Oops .. I should have noticed that...
I will try to commit again with this fix . Thanks for looking at this aqjune!!

[efriedma]

I think the point of the hasOneUse check is to avoid a possible miscompile; if a FREEZE has more than one use, all users need to see the same value. So not sure dropping the check is correct in general.

[nlopes]

In D144903#4182498, @efriedma wrote:

I think the point of the hasOneUse check is to avoid a possible miscompile; if a FREEZE has more than one use, all users need to see the same value. So not sure dropping the check is correct in general.

Good point.
This patch is not correct, must be reverted.
The shufflevectors create copies of a same 'freeze poison'. These copies must see the same value, and that's not the case after this patch.

[ManuelJBrito]

Reverted. I'll reassess what can be done here.

clang/test/CodeGen/X86/avx-cast-builtins.c

 // RUN: %clang_cc1 %s -O3 -flax-vector-conversions=none -ffreestanding %s -triple=x86_64-unknown-unknown -target-feature +avx -target-feature +avx512f  -target-feature +avx512fp16 -S -o - | FileCheck %s
 
 
 #include <immintrin.h>
 
 __m256d test_mm256_castpd128_pd256(__m128d A) {
   // CHECK-LABEL: test_mm256_castpd128_pd256
   // CHECK:         # %bb.0:
   // CHECK-NEXT:    # kill: def $xmm0 killed $xmm0 def $ymm0
__m256h test_mm256_castph128_ph256(__m128h A) {
   // CHECK-NEXT:    # kill: def $xmm0 killed $xmm0 def $ymm0
   // CHECK-NEXT:    ret{{[l|q]}}
   return _mm256_castph128_ph256(A);
 }
 
 __m512h test_mm512_castph128_ph512(__m128h A) {
   // CHECK-LABEL: test_mm512_castph128_ph512
   // CHECK:         # %bb.0:
-  // CHECK-NEXT:    # kill: def $xmm0 killed $xmm0 def $ymm0
-  // CHECK-NEXT:    vinsertf128 $1, %xmm0, %ymm0, %ymm1
-  // CHECK-NEXT:    vinsertf128 $1, %xmm0, %ymm0, %ymm0
-  // CHECK-NEXT:    vinsertf64x4 $1, %ymm1, %zmm0, %zmm0
+  // CHECK-NEXT:    # kill: def $xmm0 killed $xmm0 def $zmm0
   // CHECK-NEXT:    ret{{[l|q]}}
   return _mm512_castph128_ph512(A);
 }
 
 __m512h test_mm512_castph256_ph512(__m256h A) {
   // CHECK-LABEL: test_mm512_castph256_ph512
   // CHECK:         # %bb.0:
   // CHECK-NEXT:    # kill: def $ymm0 killed $ymm0 def $zmm0
__m512 test_mm512_castps256_ps512(__m256 A){
   // CHECK-NEXT:    # kill: def $ymm0 killed $ymm0 def $zmm0
   // CHECK-NEXT:    ret{{[l|q]}}
   return _mm512_castps256_ps512(A);
 }
 
 __m512d test_mm512_castpd128_pd512(__m128d A){
   // CHECK-LABEL: test_mm512_castpd128_pd512
   // CHECK:         # %bb.0:
-  // CHECK-NEXT:    # kill: def $xmm0 killed $xmm0 def $ymm0
-  // CHECK-NEXT:    vinsertf128 $1, %xmm0, %ymm0, %ymm1
-  // CHECK-NEXT:    vinsertf128 $1, %xmm0, %ymm0, %ymm0
-  // CHECK-NEXT:    vinsertf64x4 $1, %ymm1, %zmm0, %zmm0
+  // CHECK-NEXT:    # kill: def $xmm0 killed $xmm0 def $zmm0
   // CHECK-NEXT:    ret{{[l|q]}}
   return _mm512_castpd128_pd512(A);
 }
 
 __m512 test_mm512_castps128_ps512(__m128 A){
   // CHECK-LABEL: test_mm512_castps128_ps512
   // CHECK:         # %bb.0:
-  // CHECK-NEXT:    # kill: def $xmm0 killed $xmm0 def $ymm0
-  // CHECK-NEXT:    vinsertf128 $1, %xmm0, %ymm0, %ymm1
-  // CHECK-NEXT:    vinsertf128 $1, %xmm0, %ymm0, %ymm0
-  // CHECK-NEXT:    vinsertf64x4 $1, %ymm1, %zmm0, %zmm0
+  // CHECK-NEXT:    # kill: def $xmm0 killed $xmm0 def $zmm0
   // CHECK-NEXT:    ret{{[l|q]}}
   return _mm512_castps128_ps512(A);
 }
 
 __m512i test_mm512_castsi128_si512(__m128i A){
   // CHECK-LABEL: test_mm512_castsi128_si512
   // CHECK:         # %bb.0:
-  // CHECK-NEXT:    # kill: def $xmm0 killed $xmm0 def $ymm0
-  // CHECK-NEXT:    vinsertf128 $1, %xmm0, %ymm0, %ymm1
-  // CHECK-NEXT:    vinsertf128 $1, %xmm0, %ymm0, %ymm0
-  // CHECK-NEXT:    vinsertf64x4 $1, %ymm1, %zmm0, %zmm0
+  // CHECK-NEXT:    # kill: def $xmm0 killed $xmm0 def $zmm0
   // CHECK-NEXT:    ret{{[l|q]}}
   return _mm512_castsi128_si512(A);
 }
 
 __m512i test_mm512_castsi256_si512(__m256i A){
   // CHECK-LABEL: test_mm512_castsi256_si512
   // CHECK:         # %bb.0:
   // CHECK-NEXT:    # kill: def $ymm0 killed $ymm0 def $zmm0
   // CHECK-NEXT:    ret{{[l|q]}}
   return _mm512_castsi256_si512(A);
 }

llvm/lib/Target/X86/X86ISelLowering.cpp

static SDValue LowerAVXCONCAT_VECTORS(SDValue Op, SelectionDAG &DAG,
   unsigned NumFreezeUndef = 0;
   unsigned NumZero = 0;
   unsigned NumNonZero = 0;
   unsigned NonZeros = 0;
   for (unsigned i = 0; i != NumOperands; ++i) {
     SDValue SubVec = Op.getOperand(i);
     if (SubVec.isUndef())
       continue;
-    if (ISD::isFreezeUndef(SubVec.getNode()) && SubVec.hasOneUse())
+    if (ISD::isFreezeUndef(SubVec.getNode()))
       ++NumFreezeUndef;
     else if (ISD::isBuildVectorAllZeros(SubVec.getNode()))
       ++NumZero;
     else {
       assert(i < sizeof(NonZeros) * CHAR_BIT); // Ensure the shift is in range.
       NonZeros |= 1 << i;
       ++NumNonZero;
     }

llvm/test/CodeGen/X86/avx512-intrinsics.ll

 }
 
 declare <8 x i64> @llvm.x86.avx512.psrlv.q.512(<8 x i64>, <8 x i64>) nounwind readnone
 
 
 define <8 x double> @test_mm256_castpd128_pd256_freeze(<2 x double> %a0) nounwind {
 ; CHECK-LABEL: test_mm256_castpd128_pd256_freeze:
 ; CHECK:       # %bb.0:
-; CHECK-NEXT:    # kill: def $xmm0 killed $xmm0 def $ymm0
-; CHECK-NEXT:    vinsertf128 $1, %xmm0, %ymm0, %ymm1
-; CHECK-NEXT:    vinsertf128 $1, %xmm0, %ymm0, %ymm0
-; CHECK-NEXT:    vinsertf64x4 $1, %ymm1, %zmm0, %zmm0
+; CHECK-NEXT:    # kill: def $xmm0 killed $xmm0 def $zmm0
 ; CHECK-NEXT:    ret{{[l|q]}}
   %a1 = freeze <2 x double> poison
   %res = shufflevector <2 x double> %a0, <2 x double> %a1, <8 x i32> <i32 0, i32 1, i32 2, i32 3, i32 2, i32 3, i32 2, i32 3>
   ret <8 x double> %res
 }
 
 
 define <8 x double> @test_mm256_castpd256_pd256_freeze(<4 x double> %a0) nounwind {
 ; CHECK-LABEL: test_mm256_castpd256_pd256_freeze:
 ; CHECK:       # %bb.0:
 ; CHECK-NEXT:    # kill: def $ymm0 killed $ymm0 def $zmm0
 ; CHECK-NEXT:    ret{{[l|q]}}
   %a1 = freeze <4 x double> poison
   %res = shufflevector <4 x double> %a0, <4 x double> %a1, <8 x i32> <i32 0, i32 1, i32 2, i32 3, i32 4, i32 5, i32 6, i32 7>
   ret <8 x double> %res
 }
 
 
 define <16 x float> @test_mm256_castps128_ps512_freeze(<4 x float> %a0) nounwind {
 ; CHECK-LABEL: test_mm256_castps128_ps512_freeze:
 ; CHECK:       # %bb.0:
-; CHECK-NEXT:    # kill: def $xmm0 killed $xmm0 def $ymm0
-; CHECK-NEXT:    vinsertf128 $1, %xmm0, %ymm0, %ymm1
-; CHECK-NEXT:    vinsertf128 $1, %xmm0, %ymm0, %ymm0
-; CHECK-NEXT:    vinsertf64x4 $1, %ymm1, %zmm0, %zmm0
+; CHECK-NEXT:    # kill: def $xmm0 killed $xmm0 def $zmm0
 ; CHECK-NEXT:    ret{{[l|q]}}
   %a1 = freeze <4 x float> poison
   %res = shufflevector <4 x float> %a0, <4 x float> %a1, <16x i32> <i32 0, i32 1, i32 2, i32 3, i32 4, i32 5, i32 6, i32 7, i32 4, i32 5, i32 6, i32 7, i32 4, i32 5, i32 6, i32 7>
   ret <16 x float> %res
 }
 
 
 define <16 x float> @test_mm256_castps256_ps512_freeze(<8 x float> %a0) nounwind {
 ; CHECK-LABEL: test_mm256_castps256_ps512_freeze:
 ; CHECK:       # %bb.0:
 ; CHECK-NEXT:    # kill: def $ymm0 killed $ymm0 def $zmm0
 ; CHECK-NEXT:    ret{{[l|q]}}
   %a1 = freeze <8 x float> poison
   %res = shufflevector <8 x float> %a0, <8 x float> %a1, <16x i32> <i32 0, i32 1, i32 2, i32 3, i32 4, i32 5, i32 6, i32 7, i32 8, i32 9, i32 10, i32 11, i32 12, i32 13, i32 14, i32 15>
   ret <16 x float> %res
 }
 
 
 define <8 x i64> @test_mm512_castsi128_si512_freeze(<2 x i64> %a0) nounwind {
 ; CHECK-LABEL: test_mm512_castsi128_si512_freeze:
 ; CHECK:       # %bb.0:
-; CHECK-NEXT:    # kill: def $xmm0 killed $xmm0 def $ymm0
-; CHECK-NEXT:    vinsertf128 $1, %xmm0, %ymm0, %ymm1
-; CHECK-NEXT:    vinsertf128 $1, %xmm0, %ymm0, %ymm0
-; CHECK-NEXT:    vinsertf64x4 $1, %ymm1, %zmm0, %zmm0
+; CHECK-NEXT:    # kill: def $xmm0 killed $xmm0 def $zmm0
 ; CHECK-NEXT:    ret{{[l|q]}}
   %a1 = freeze <2 x i64> poison
   %res = shufflevector <2 x i64> %a0, <2 x i64> %a1, <8 x i32> <i32 0, i32 1, i32 2, i32 3, i32 2, i32 3, i32 2, i32 3>
   ret <8 x i64> %res
 }
 
 
 define <8 x i64> @test_mm512_castsi256_si512_pd256_freeze(<4 x i64> %a0) nounwind {

llvm/test/CodeGen/X86/avx512fp16-intrinsics.ll

; CHECK-NEXT:    retq
   %res = shufflevector <8 x half> %a0, <8 x half> %a1, <16 x i32> <i32 0, i32 1, i32 2, i32 3, i32 4, i32 5, i32 6, i32 7, i32 8, i32 9, i32 10, i32 11, i32 12, i32 13, i32 14, i32 15>
   ret <16 x half> %res
 }
 
 
 define <32 x half> @test_mm512_castph128_ph512_freeze(<8 x half> %a0) nounwind {
 ; CHECK-LABEL: test_mm512_castph128_ph512_freeze:
 ; CHECK:       # %bb.0:
-; CHECK-NEXT:    # kill: def $xmm0 killed $xmm0 def $ymm0
-; CHECK-NEXT:    vinsertf128 $1, %xmm0, %ymm0, %ymm1
-; CHECK-NEXT:    vinsertf128 $1, %xmm0, %ymm0, %ymm0
-; CHECK-NEXT:    vinsertf64x4 $1, %ymm1, %zmm0, %zmm0
+; CHECK-NEXT:    # kill: def $xmm0 killed $xmm0 def $zmm0
 ; CHECK-NEXT:    retq
   %a1 = freeze <8 x half> poison
   %res = shufflevector <8 x half> %a0, <8 x half> %a1, <32 x i32> <i32 0, i32 1, i32 2, i32 3, i32 4, i32 5, i32 6, i32 7, i32 8, i32 9, i32 10, i32 11, i32 12, i32 13, i32 14, i32 15, i32 8, i32 9, i32 10, i32 11, i32 12, i32 13, i32 14, i32 15, i32 8, i32 9, i32 10, i32 11, i32 12, i32 13, i32 14, i32 15>
   ret <32 x half> %res
 }
 
 
 define <32 x half> @test_mm512_castph256_ph512_freeze(<16 x half> %a0) nounwind {
 ; CHECK-LABEL: test_mm512_castph256_ph512_freeze:
 ; CHECK:       # %bb.0:
 ; CHECK-NEXT:    # kill: def $ymm0 killed $ymm0 def $zmm0
 ; CHECK-NEXT:    retq
   %a1 = freeze <16 x half> poison
   %res = shufflevector <16 x half> %a0, <16 x half> %a1, <32 x i32> <i32 0, i32 1, i32 2, i32 3, i32 4, i32 5, i32 6, i32 7, i32 8, i32 9, i32 10, i32 11, i32 12, i32 13, i32 14, i32 15, i32 16, i32 17, i32 18, i32 19, i32 20, i32 21, i32 22, i32 23, i32 24, i32 25, i32 26, i32 27, i32 28, i32 29, i32 30, i32 31>
   ret <32 x half> %res
 }