[X86] Always assign reassoc flag for intrinsics *reduce_add/mul_ps/pd.

[pengfei]

Summary

Intrinsics *reduce_add/mul_ps/pd have assumption that the elements in
the vector are reassociable. So we need to always assign the reassoc
flag when we call _mm_reduce_* intrinsics.

[spatel]

clang/lib/CodeGen/CGBuiltin.cpp

13829

I haven't looked at this part of the compiler in a long time, so I was wondering how we handle FMF scope. It looks like there is already an FMFGuard object in place -- CodeGenFunction::CGFPOptionsRAII(). So setting FMF here will not affect anything but this CreateCall().

Does that match your understanding? Should we have an extra regression test to make sure that does not change?

I am imagining something like:

double test_mm512_reduce_add_pd(__m512d __W, double ExtraAddOp) {
  double S = _mm512_reduce_add_pd(__W) + ExtraAddOp;
  return S;
}

Then we could confirm that reassoc is not applied to the fadd that follows the reduction call.

13829

Currently (and we could say that this is an LLVM codegen bug), we will not generate the optimal/expected reduction with reassoc alone.

I think the x86 reduction definition is implicitly assuming that -0.0 is not meaningful here, so we should add nsz too.

The backend is expecting an explicit nsz on this op. Ie, I see this x86 asm currently with only reassoc:

	vextractf64x4	$1, %zmm0, %ymm1
	vaddpd	%zmm1, %zmm0, %zmm0
	vextractf128	$1, %ymm0, %xmm1
	vaddpd	%xmm1, %xmm0, %xmm0
	vpermilpd	$1, %xmm0, %xmm1      
	vaddsd	%xmm1, %xmm0, %xmm0 
	vxorpd	%xmm1, %xmm1, %xmm1   <--- create 0.0
	vaddsd	%xmm1, %xmm0, %xmm0   <--- add it to the reduction result

Alternatively (and I'm not sure where it is specified), we could replace the default 0.0 argument with -0.0?

clang/lib/Headers/avx512fintrin.h

9300

This is an existing text bug, but if we are changing this text, we might as well fix it in this patch - I'm not sure what "off" refers to here. Should that be "order"?

9303

Typo: "floating-point types"

9304

Also mention that sign of zero is indeterminate. We might use the LangRef text as a model for what to say here:
https://llvm.org/docs/LangRef.html#llvm-vector-reduce-fadd-intrinsic

[spatel]

clang/lib/Headers/avx512fintrin.h

9355

Ah - this is where the +0.0 is specified. This should be -0.0. We could still add 'nsz' flag to be safe.

9365

This also should be changed to -0.0?

[pengfei]

Address Sanjay's comments. Thanks for the thoroughly review!

clang/lib/CodeGen/CGBuiltin.cpp

13829

Confirmed by new tests.

13829

I think there's no such assumption for fadd/fmul instructions. We do have it for fmin/fmax. So I think we don't need to add nsz here.

clang/lib/Headers/avx512fintrin.h

9304

Got it. Thanks!

9355

-0.0 can fix the problem. But we don't need to add 'nsz'. We can add it if we can find a corner case.

[spatel]

LGTM

clang/lib/CodeGen/CGBuiltin.cpp

case X86::BI__builtin_ia32_reduce_and_q512: {
     Function *F =
         CGM.getIntrinsic(Intrinsic::vector_reduce_and, Ops[0]->getType());
     return Builder.CreateCall(F, {Ops[0]});
   }
   case X86::BI__builtin_ia32_reduce_fadd_pd512:
   case X86::BI__builtin_ia32_reduce_fadd_ps512: {
     Function *F =
         CGM.getIntrinsic(Intrinsic::vector_reduce_fadd, Ops[1]->getType());
+    Builder.getFastMathFlags().setAllowReassoc(true);
     return Builder.CreateCall(F, {Ops[0], Ops[1]});
   }
   case X86::BI__builtin_ia32_reduce_fmul_pd512:
   case X86::BI__builtin_ia32_reduce_fmul_ps512: {
     Function *F =
         CGM.getIntrinsic(Intrinsic::vector_reduce_fmul, Ops[1]->getType());
+    Builder.getFastMathFlags().setAllowReassoc(true);
     return Builder.CreateCall(F, {Ops[0], Ops[1]});
   }
   case X86::BI__builtin_ia32_reduce_mul_d512:
   case X86::BI__builtin_ia32_reduce_mul_q512: {
     Function *F =
         CGM.getIntrinsic(Intrinsic::vector_reduce_mul, Ops[0]->getType());
     return Builder.CreateCall(F, {Ops[0]});
   }

clang/lib/Headers/avx512fintrin.h

 static __inline__ __m512d __DEFAULT_FN_ATTRS512
 _mm512_mask_abs_pd(__m512d __W, __mmask8 __K, __m512d __A)
 {
   return (__m512d)_mm512_mask_and_epi64((__v8di)__W, __K, _mm512_set1_epi64(0x7FFFFFFFFFFFFFFF),(__v8di)__A);
 }
 
 /* Vector-reduction arithmetic accepts vectors as inputs and produces scalars as
  * outputs. This class of vector operation forms the basis of many scientific
- * computations. In vector-reduction arithmetic, the evaluation off is
+ * computations. In vector-reduction arithmetic, the evaluation order is
  * independent of the order of the input elements of V.
 
+ * For floating point types, we always assume the elements are reassociable even
+ * if -fast-math is off.
+
  * Used bisection method. At each step, we partition the vector with previous
  * step in half, and the operation is performed on its two halves.
  * This takes log2(n) steps where n is the number of elements in the vector.
  */
 
 static __inline__ long long __DEFAULT_FN_ATTRS512 _mm512_reduce_add_epi64(__m512i __W) {
   return __builtin_ia32_reduce_add_q512(__W);
 }

 }
 
 static __inline__ long long __DEFAULT_FN_ATTRS512
 _mm512_mask_reduce_or_epi64(__mmask8 __M, __m512i __W) {
   __W = _mm512_maskz_mov_epi64(__M, __W);
   return __builtin_ia32_reduce_or_q512(__W);
 }
 
+// -0.0 is used to ignore the start value since it is the neutral value of
+// floating point addition. For more information, please refer to
+// https://llvm.org/docs/LangRef.html#llvm-vector-reduce-fadd-intrinsic
 static __inline__ double __DEFAULT_FN_ATTRS512 _mm512_reduce_add_pd(__m512d __W) {
-  return __builtin_ia32_reduce_fadd_pd512(0.0, __W);
+  return __builtin_ia32_reduce_fadd_pd512(-0.0, __W);
 }
 
 static __inline__ double __DEFAULT_FN_ATTRS512 _mm512_reduce_mul_pd(__m512d __W) {
   return __builtin_ia32_reduce_fmul_pd512(1.0, __W);
 }
 
 static __inline__ double __DEFAULT_FN_ATTRS512
 _mm512_mask_reduce_add_pd(__mmask8 __M, __m512d __W) {
   __W = _mm512_maskz_mov_pd(__M, __W);
-  return __builtin_ia32_reduce_fadd_pd512(0.0, __W);
+  return __builtin_ia32_reduce_fadd_pd512(-0.0, __W);
 }
 
 static __inline__ double __DEFAULT_FN_ATTRS512
 _mm512_mask_reduce_mul_pd(__mmask8 __M, __m512d __W) {
   __W = _mm512_mask_mov_pd(_mm512_set1_pd(1.0), __M, __W);
   return __builtin_ia32_reduce_fmul_pd512(1.0, __W);
 }
 

 static __inline__ int __DEFAULT_FN_ATTRS512
 _mm512_mask_reduce_or_epi32(__mmask16 __M, __m512i __W) {
   __W = _mm512_maskz_mov_epi32(__M, __W);
   return __builtin_ia32_reduce_or_d512((__v16si)__W);
 }
 
 static __inline__ float __DEFAULT_FN_ATTRS512
 _mm512_reduce_add_ps(__m512 __W) {
-  return __builtin_ia32_reduce_fadd_ps512(0.0f, __W);
+  return __builtin_ia32_reduce_fadd_ps512(-0.0f, __W);
 }
 
 static __inline__ float __DEFAULT_FN_ATTRS512
 _mm512_reduce_mul_ps(__m512 __W) {
   return __builtin_ia32_reduce_fmul_ps512(1.0f, __W);
 }
 
 static __inline__ float __DEFAULT_FN_ATTRS512
 _mm512_mask_reduce_add_ps(__mmask16 __M, __m512 __W) {
   __W = _mm512_maskz_mov_ps(__M, __W);
-  return __builtin_ia32_reduce_fadd_ps512(0.0f, __W);
+  return __builtin_ia32_reduce_fadd_ps512(-0.0f, __W);
 }
 
 static __inline__ float __DEFAULT_FN_ATTRS512
 _mm512_mask_reduce_mul_ps(__mmask16 __M, __m512 __W) {
   __W = _mm512_mask_mov_ps(_mm512_set1_ps(1.0f), __M, __W);
   return __builtin_ia32_reduce_fmul_ps512(1.0f, __W);
 }
 

clang/test/CodeGen/X86/avx512-reduceIntrin.c

 // RUN: %clang_cc1 -ffreestanding %s -O0 -triple=x86_64-apple-darwin -target-cpu skylake-avx512 -emit-llvm -o - -Wall -Werror | FileCheck %s
 
 #include <immintrin.h>
 
 long long test_mm512_reduce_add_epi64(__m512i __W){
 // CHECK-LABEL: @test_mm512_reduce_add_epi64(
 // CHECK:    call i64 @llvm.vector.reduce.add.v8i64(<8 x i64> %{{.*}})
   return _mm512_reduce_add_epi64(__W);
 }
 
 long long test_mm512_reduce_mul_epi64(__m512i __W){
 // CHECK-LABEL: @test_mm512_reduce_mul_epi64(
 // CHECK:    call i64 @llvm.vector.reduce.mul.v8i64(<8 x i64> %{{.*}})
-  return _mm512_reduce_mul_epi64(__W); 
+  return _mm512_reduce_mul_epi64(__W);
 }
 
 long long test_mm512_reduce_or_epi64(__m512i __W){
 // CHECK-LABEL: @test_mm512_reduce_or_epi64(
 // CHECK:    call i64 @llvm.vector.reduce.or.v8i64(<8 x i64> %{{.*}})
-  return _mm512_reduce_or_epi64(__W); 
+  return _mm512_reduce_or_epi64(__W);
 }
 
 long long test_mm512_reduce_and_epi64(__m512i __W){
 // CHECK-LABEL: @test_mm512_reduce_and_epi64(
 // CHECK:    call i64 @llvm.vector.reduce.and.v8i64(<8 x i64> %{{.*}})
   return _mm512_reduce_and_epi64(__W);
 }
 
 long long test_mm512_mask_reduce_add_epi64(__mmask8 __M, __m512i __W){
 // CHECK-LABEL: @test_mm512_mask_reduce_add_epi64(
 // CHECK:    bitcast i8 %{{.*}} to <8 x i1>
 // CHECK:    select <8 x i1> %{{.*}}, <8 x i64> %{{.*}}, <8 x i64> %{{.*}}
 // CHECK:    call i64 @llvm.vector.reduce.add.v8i64(<8 x i64> %{{.*}})
-  return _mm512_mask_reduce_add_epi64(__M, __W); 
+  return _mm512_mask_reduce_add_epi64(__M, __W);
 }
 
 long long test_mm512_mask_reduce_mul_epi64(__mmask8 __M, __m512i __W){
 // CHECK-LABEL: @test_mm512_mask_reduce_mul_epi64(
 // CHECK:    bitcast i8 %{{.*}} to <8 x i1>
 // CHECK:    select <8 x i1> %{{.*}}, <8 x i64> %{{.*}}, <8 x i64> %{{.*}}
 // CHECK:    call i64 @llvm.vector.reduce.mul.v8i64(<8 x i64> %{{.*}})
-  return _mm512_mask_reduce_mul_epi64(__M, __W); 
+  return _mm512_mask_reduce_mul_epi64(__M, __W);
 }
 
 long long test_mm512_mask_reduce_and_epi64(__mmask8 __M, __m512i __W){
 // CHECK-LABEL: @test_mm512_mask_reduce_and_epi64(
 // CHECK:    bitcast i8 %{{.*}} to <8 x i1>
 // CHECK:    select <8 x i1> %{{.*}}, <8 x i64> %{{.*}}, <8 x i64> %{{.*}}
 // CHECK:    call i64 @llvm.vector.reduce.and.v8i64(<8 x i64> %{{.*}})
-  return _mm512_mask_reduce_and_epi64(__M, __W); 
+  return _mm512_mask_reduce_and_epi64(__M, __W);
 }
 
 long long test_mm512_mask_reduce_or_epi64(__mmask8 __M, __m512i __W){
 // CHECK-LABEL: @test_mm512_mask_reduce_or_epi64(
 // CHECK:    bitcast i8 %{{.*}} to <8 x i1>
 // CHECK:    select <8 x i1> %{{.*}}, <8 x i64> %{{.*}}, <8 x i64> %{{.*}}
 // CHECK:    call i64 @llvm.vector.reduce.or.v8i64(<8 x i64> %{{.*}})
-  return _mm512_mask_reduce_or_epi64(__M, __W); 
+  return _mm512_mask_reduce_or_epi64(__M, __W);
 }
 
 int test_mm512_reduce_add_epi32(__m512i __W){
 // CHECK-LABEL: @test_mm512_reduce_add_epi32(
 // CHECK:    call i32 @llvm.vector.reduce.add.v16i32(<16 x i32> %{{.*}})
-  return _mm512_reduce_add_epi32(__W); 
+  return _mm512_reduce_add_epi32(__W);
 }
 
 int test_mm512_reduce_mul_epi32(__m512i __W){
 // CHECK-LABEL: @test_mm512_reduce_mul_epi32(
 // CHECK:    call i32 @llvm.vector.reduce.mul.v16i32(<16 x i32> %{{.*}})
-  return _mm512_reduce_mul_epi32(__W); 
+  return _mm512_reduce_mul_epi32(__W);
 }
 
 int test_mm512_reduce_or_epi32(__m512i __W){
 // CHECK:    call i32 @llvm.vector.reduce.or.v16i32(<16 x i32> %{{.*}})
-  return _mm512_reduce_or_epi32(__W); 
+  return _mm512_reduce_or_epi32(__W);
 }
 
 int test_mm512_reduce_and_epi32(__m512i __W){
 // CHECK-LABEL: @test_mm512_reduce_and_epi32(
 // CHECK:    call i32 @llvm.vector.reduce.and.v16i32(<16 x i32> %{{.*}})
-  return _mm512_reduce_and_epi32(__W); 
+  return _mm512_reduce_and_epi32(__W);
 }
 
 int test_mm512_mask_reduce_add_epi32(__mmask16 __M, __m512i __W){
 // CHECK-LABEL: @test_mm512_mask_reduce_add_epi32(
 // CHECK:    bitcast i16 %{{.*}} to <16 x i1>
 // CHECK:    select <16 x i1> %{{.*}}, <16 x i32> %{{.*}}, <16 x i32> %{{.*}}
 // CHECK:    call i32 @llvm.vector.reduce.add.v16i32(<16 x i32> %{{.*}})
-  return _mm512_mask_reduce_add_epi32(__M, __W); 
+  return _mm512_mask_reduce_add_epi32(__M, __W);
 }
 
 int test_mm512_mask_reduce_mul_epi32(__mmask16 __M, __m512i __W){
 // CHECK-LABEL: @test_mm512_mask_reduce_mul_epi32(
 // CHECK:    bitcast i16 %{{.*}} to <16 x i1>
 // CHECK:    select <16 x i1> %{{.*}}, <16 x i32> %{{.*}}, <16 x i32> %{{.*}}
 // CHECK:    call i32 @llvm.vector.reduce.mul.v16i32(<16 x i32> %{{.*}})
-  return _mm512_mask_reduce_mul_epi32(__M, __W); 
+  return _mm512_mask_reduce_mul_epi32(__M, __W);
 }
 
 int test_mm512_mask_reduce_and_epi32(__mmask16 __M, __m512i __W){
 // CHECK-LABEL: @test_mm512_mask_reduce_and_epi32(
 // CHECK:    bitcast i16 %{{.*}} to <16 x i1>
 // CHECK:    select <16 x i1> %{{.*}}, <16 x i32> %{{.*}}, <16 x i32> %{{.*}}
 // CHECK:    call i32 @llvm.vector.reduce.and.v16i32(<16 x i32> %{{.*}})
-  return _mm512_mask_reduce_and_epi32(__M, __W); 
+  return _mm512_mask_reduce_and_epi32(__M, __W);
 }
 
 int test_mm512_mask_reduce_or_epi32(__mmask16 __M, __m512i __W){
 // CHECK-LABEL: @test_mm512_mask_reduce_or_epi32(
 // CHECK:    bitcast i16 %{{.*}} to <16 x i1>
 // CHECK:    select <16 x i1> %{{.*}}, <16 x i32> %{{.*}}, <16 x i32> %{{.*}}
 // CHECK:    call i32 @llvm.vector.reduce.or.v16i32(<16 x i32> %{{.*}})
-  return _mm512_mask_reduce_or_epi32(__M, __W); 
+  return _mm512_mask_reduce_or_epi32(__M, __W);
 }
 
-double test_mm512_reduce_add_pd(__m512d __W){
+double test_mm512_reduce_add_pd(__m512d __W, double ExtraAddOp){
 // CHECK-LABEL: @test_mm512_reduce_add_pd(
-// CHECK:    call double @llvm.vector.reduce.fadd.v8f64(double 0.000000e+00, <8 x double> %{{.*}})
-  return _mm512_reduce_add_pd(__W); 
+// CHECK-NOT: reassoc
+// CHECK:    call reassoc double @llvm.vector.reduce.fadd.v8f64(double -0.000000e+00, <8 x double> %{{.*}})
+// CHECK-NOT: reassoc
+  return _mm512_reduce_add_pd(__W) + ExtraAddOp;
 }
 
-double test_mm512_reduce_mul_pd(__m512d __W){
+double test_mm512_reduce_mul_pd(__m512d __W, double ExtraMulOp){
 // CHECK-LABEL: @test_mm512_reduce_mul_pd(
-// CHECK:    call double @llvm.vector.reduce.fmul.v8f64(double 1.000000e+00, <8 x double> %{{.*}})
-  return _mm512_reduce_mul_pd(__W); 
+// CHECK-NOT: reassoc
+// CHECK:    call reassoc double @llvm.vector.reduce.fmul.v8f64(double 1.000000e+00, <8 x double> %{{.*}})
+// CHECK-NOT: reassoc
+  return _mm512_reduce_mul_pd(__W) * ExtraMulOp;
 }
 
 float test_mm512_reduce_add_ps(__m512 __W){
 // CHECK-LABEL: @test_mm512_reduce_add_ps(
-// CHECK:    call float @llvm.vector.reduce.fadd.v16f32(float 0.000000e+00, <16 x float> %{{.*}})
-  return _mm512_reduce_add_ps(__W); 
+// CHECK:    call reassoc float @llvm.vector.reduce.fadd.v16f32(float -0.000000e+00, <16 x float> %{{.*}})
+  return _mm512_reduce_add_ps(__W);
 }
 
 float test_mm512_reduce_mul_ps(__m512 __W){
 // CHECK-LABEL: @test_mm512_reduce_mul_ps(
-// CHECK:    call float @llvm.vector.reduce.fmul.v16f32(float 1.000000e+00, <16 x float> %{{.*}})
-  return _mm512_reduce_mul_ps(__W); 
+// CHECK:    call reassoc float @llvm.vector.reduce.fmul.v16f32(float 1.000000e+00, <16 x float> %{{.*}})
+  return _mm512_reduce_mul_ps(__W);
 }
 
 double test_mm512_mask_reduce_add_pd(__mmask8 __M, __m512d __W){
 // CHECK-LABEL: @test_mm512_mask_reduce_add_pd(
 // CHECK:    bitcast i8 %{{.*}} to <8 x i1>
 // CHECK:    select <8 x i1> %{{.*}}, <8 x double> %{{.*}}, <8 x double> %{{.*}}
-// CHECK:    call double @llvm.vector.reduce.fadd.v8f64(double 0.000000e+00, <8 x double> %{{.*}})
-  return _mm512_mask_reduce_add_pd(__M, __W); 
+// CHECK:    call reassoc double @llvm.vector.reduce.fadd.v8f64(double -0.000000e+00, <8 x double> %{{.*}})
+  return _mm512_mask_reduce_add_pd(__M, __W);
 }
 
 double test_mm512_mask_reduce_mul_pd(__mmask8 __M, __m512d __W){
 // CHECK-LABEL: @test_mm512_mask_reduce_mul_pd(
 // CHECK:    bitcast i8 %{{.*}} to <8 x i1>
 // CHECK:    select <8 x i1> %{{.*}}, <8 x double> %{{.*}}, <8 x double> %{{.*}}
-// CHECK:    call double @llvm.vector.reduce.fmul.v8f64(double 1.000000e+00, <8 x double> %{{.*}})
-  return _mm512_mask_reduce_mul_pd(__M, __W); 
+// CHECK:    call reassoc double @llvm.vector.reduce.fmul.v8f64(double 1.000000e+00, <8 x double> %{{.*}})
+  return _mm512_mask_reduce_mul_pd(__M, __W);
 }
 
 float test_mm512_mask_reduce_add_ps(__mmask16 __M, __m512 __W){
 // CHECK-LABEL: @test_mm512_mask_reduce_add_ps(
 // CHECK:    bitcast i16 %{{.*}} to <16 x i1>
 // CHECK:    select <16 x i1> %{{.*}}, <16 x float> {{.*}}, <16 x float> {{.*}}
-// CHECK:    call float @llvm.vector.reduce.fadd.v16f32(float 0.000000e+00, <16 x float> %{{.*}})
-  return _mm512_mask_reduce_add_ps(__M, __W); 
+// CHECK:    call reassoc float @llvm.vector.reduce.fadd.v16f32(float -0.000000e+00, <16 x float> %{{.*}})
+  return _mm512_mask_reduce_add_ps(__M, __W);
 }
 
 float test_mm512_mask_reduce_mul_ps(__mmask16 __M, __m512 __W){
 // CHECK-LABEL: @test_mm512_mask_reduce_mul_ps(
 // CHECK:    bitcast i16 %{{.*}} to <16 x i1>
 // CHECK:    select <16 x i1> %{{.*}}, <16 x float> {{.*}}, <16 x float> %{{.*}}
-// CHECK:    call float @llvm.vector.reduce.fmul.v16f32(float 1.000000e+00, <16 x float> %{{.*}})
-  return _mm512_mask_reduce_mul_ps(__M, __W); 
+// CHECK:    call reassoc float @llvm.vector.reduce.fmul.v16f32(float 1.000000e+00, <16 x float> %{{.*}})
+  return _mm512_mask_reduce_mul_ps(__M, __W);
 }