[X86][Reduce] Preserve fast math flags when change it. NFCI

[pengfei]

Summary

@arsenm raised a good question that we should use a flag guard.
But I found it is not a problem as long as user uses intrinsics only: https://godbolt.org/z/WoYsqqjh3
Anyway, it is still nice to have.

[barannikov88]

clang/lib/CodeGen/CGBuiltin.cpp

14743

Could be just:

return Builder.CreateCall(F, {Ops[0], Ops[1]})
    ->getFastMathFlags()
    .setAllowReassoc();

so that you don't have to save and restore Builder's flags.

[barannikov88]

clang/lib/CodeGen/CGBuiltin.cpp

14743

No, it couldn't be, just tried :(

[foad]

clang/lib/CodeGen/CGBuiltin.cpp

14742

We have FastMathFlagGuard for automatically saving and restoring fast math flags.

[arsenm]

Needs tests. I couldn’t find any for the base builtins either

[pengfei]

Use FastMathFlagGuard instead, thanks @foad!

In D140467#4010296, @arsenm wrote:

Needs tests. I couldn’t find any for the base builtins either

I don't think so. We have tested their intrinsic wrappers, e.g., https://github.com/llvm/llvm-project/blob/main/clang/test/CodeGen/X86/avx512fp16-builtins.c#L4451

We don't need to test those target specific builtins. They are neither guaranteed to be cross compiler compatible nor release by release.

Besides, we have thousands of intrinsics in X86. Adding their builtin tests have no any benefits and increase the time in lit tests.

clang/lib/CodeGen/CGBuiltin.cpp

14743

Thanks for trying it :)

[arsenm]

In D140467#4010378, @pengfei wrote:

Use FastMathFlagGuard instead, thanks @foad!

In D140467#4010296, @arsenm wrote:

Needs tests. I couldn’t find any for the base builtins either

I don't think so. We have tested their intrinsic wrappers, e.g., https://github.com/llvm/llvm-project/blob/main/clang/test/CodeGen/X86/avx512fp16-builtins.c#L4451

These are not testing use of these builtins correctly

We don't need to test those target specific builtins. They are neither guaranteed to be cross compiler compatible nor release by release.

If it exists it must be tested.

Besides, we have thousands of intrinsics in X86. Adding their builtin tests have no any benefits and increase the time in lit tests.

Every piece of code generation needs to be tested.

[arsenm]

In D140467#4010507, @arsenm wrote:

In D140467#4010378, @pengfei wrote:

Use FastMathFlagGuard instead, thanks @foad!

In D140467#4010296, @arsenm wrote:

Needs tests. I couldn’t find any for the base builtins either

I don't think so. We have tested their intrinsic wrappers, e.g., https://github.com/llvm/llvm-project/blob/main/clang/test/CodeGen/X86/avx512fp16-builtins.c#L4451

These are not testing use of these builtins correctly

This test amounts to a builtin header test for immintrin.h. This should move to clang/test/Headers and replaced with a builtin test directly checking the builtin handling

[pengfei]

If it exists it must be tested.
Every piece of code generation needs to be tested.

Let me show you the number:

$ grep -rho '__builtin_ia32\w\+' clang/test/CodeGen | sort|uniq |wc -l
337
$ grep -rho '_mm512_\w\+' clang/test/CodeGen | sort|uniq |wc -l
2304

Note most of __builtin_ia32 tests are negative ones and _mm512_ is less than 1/3 of the total x86 intrinsics. Two orders of magnitude!

I took a look at the tests. The positive tests come from clang/test/CodeGen/builtins-x86.c. Other targets don't have a big test either
wc -l clang/test/CodeGen/builtins-*

These are not testing use of these builtins correctly

As I have explained, users are not suggested to use these builtins given we have provided the more stable, well documented corresponding intrinsics. The only case user has to use it is the intrinsic is missing. In that case, we do need test case for it.

In a word, as titled, it is NFC from the perspective of intrinsics. So I think we don't need test cases for them.

This test amounts to a builtin header test for immintrin.h. This should move to clang/test/Headers and replaced with a builtin test directly checking the builtin handling

Not get your point. But currently no builtin tests under clang/test/Headers/.

[lebedev.ri]

In D140467#4010675, @pengfei wrote:

If it exists it must be tested.
Every piece of code generation needs to be tested.

Let me show you the number:

$ grep -rho '__builtin_ia32\w\+' clang/test/CodeGen | sort|uniq |wc -l
337
$ grep -rho '_mm512_\w\+' clang/test/CodeGen | sort|uniq |wc -l
2304

Note most of __builtin_ia32 tests are negative ones and _mm512_ is less than 1/3 of the total x86 intrinsics. Two orders of magnitude!

I took a look at the tests. The positive tests come from clang/test/CodeGen/builtins-x86.c. Other targets don't have a big test either
wc -l clang/test/CodeGen/builtins-*

These are not testing use of these builtins correctly

As I have explained, users are not suggested to use these builtins given we have provided the more stable, well documented corresponding intrinsics. The only case user has to use it is the intrinsic is missing. In that case, we do need test case for it.

In a word, as titled, it is NFC from the perspective of intrinsics. So I think we don't need test cases for them.

This test amounts to a builtin header test for immintrin.h. This should move to clang/test/Headers and replaced with a builtin test directly checking the builtin handling

Not get your point. But currently no builtin tests under clang/test/Headers/.

I agree with @arsenm. At least for clang irgen, we should have good test coverage.

[arsenm]

In D140467#4010675, @pengfei wrote:

As I have explained, users are not suggested to use these builtins given we have provided the more stable, well documented corresponding intrinsics. The only case user has to use it is the intrinsic is missing. In that case, we do need test case for it.

Tests don't exist for users, they exist for compiler developers. We shouldn't have frail infrastructure that depends on people not using things that are available. I'm not asking to test every builtin in this change, but this does need a test that shows a case where fast math flags were incorrectly applied. The broader issue of missing builtin coverage is a separate problem.

In a word, as titled, it is NFC from the perspective of intrinsics. So I think we don't need test cases for them.

This is not NFC. This changes codegen.

This test amounts to a builtin header test for immintrin.h. This should move to clang/test/Headers and replaced with a builtin test directly checking the builtin handling

Not get your point. But currently no builtin tests under clang/test/Headers/.

These "builtin tests" are checking wrappers implemented in a builtin header, not builtins. These are two levels of functionality that should be independently tested

[pengfei]

Add test case to check FastMathFlagGuard works.

Tests don't exist for users, they exist for compiler developers...
I agree with @arsenm. At least for clang irgen, we should have good test coverage.

You are right. Added a test case.

This test amounts to a builtin header test for immintrin.h. This should move to clang/test/Headers and replaced with a builtin test directly checking the builtin handling

I got your point now, thanks! However, I don't agree with you. Although most intrinsics are simple wrappers of builtins, we have many intrinsics are combinations of more builtins. It's common in mask intrinsics. We also have intrinsics that permute their arguments order when calling to builtins.
IIUC, -fsyntax-only only checks for types, builtins existence. It even doesn't check if intrinsics mapped to the right builtins. Not to mention above cases. So it doesn't sound feasible to me.

[arsenm]

In D140467#4013107, @pengfei wrote:

Add test case to check FastMathFlagGuard works.

Not to mention above cases. So it doesn't sound feasible to me.

Testing is always feasible. You could even just generate all the combinations

clang/lib/CodeGen/CGBuiltin.cpp

13116

Should only do this in the cases that actually set the flags

clang/test/CodeGen/builtins-x86-reduce.c

8

Should test the builtins from both sets

9–11

use update_cc_test_checks, check-not is super fragile

[pengfei]

Address review comments. Thanks!

clang/test/CodeGen/builtins-x86-reduce.c

8

Do you mean this?

[arsenm]

clang/test/CodeGen/builtins-x86-reduce.c

8

Almost. You added the guard to 4 switch cases, so I would expect at minimum one builtin from each of the cases. These are both the same builtin. You also should have an fmul, fmin and fmax case

[pengfei]

Add fmul, fmin and fmax cases.

clang/lib/CodeGen/CGBuiltin.cpp

auto getVectorFCmpIR = [this, &Ops, E](CmpInst::Predicate Pred,
     else
       Cmp = Builder.CreateFCmp(Pred, Ops[0], Ops[1]);
     llvm::VectorType *FPVecTy = cast<llvm::VectorType>(Ops[0]->getType());
     llvm::VectorType *IntVecTy = llvm::VectorType::getInteger(FPVecTy);
     Value *Sext = Builder.CreateSExt(Cmp, IntVecTy);
     return Builder.CreateBitCast(Sext, FPVecTy);
   };
 
+  IRBuilder<>::FastMathFlagGuard FMFGuard(Builder);
+
   switch (BuiltinID) {
   default: return nullptr;
   case X86::BI_mm_prefetch: {
     Value *Address = Ops[0];
     ConstantInt *C = cast<ConstantInt>(Ops[1]);
     Value *RW = ConstantInt::get(Int32Ty, (C->getZExtValue() >> 2) & 0x1);
     Value *Locality = ConstantInt::get(Int32Ty, C->getZExtValue() & 0x3);
     Value *Data = ConstantInt::get(Int32Ty, 1);

   // Reductions
   case X86::BI__builtin_ia32_reduce_fadd_pd512:
   case X86::BI__builtin_ia32_reduce_fadd_ps512:
   case X86::BI__builtin_ia32_reduce_fadd_ph512:
   case X86::BI__builtin_ia32_reduce_fadd_ph256:
   case X86::BI__builtin_ia32_reduce_fadd_ph128: {
     Function *F =
         CGM.getIntrinsic(Intrinsic::vector_reduce_fadd, Ops[1]->getType());
     Builder.getFastMathFlags().setAllowReassoc();
     return Builder.CreateCall(F, {Ops[0], Ops[1]});
   }
   case X86::BI__builtin_ia32_reduce_fmul_pd512:
   case X86::BI__builtin_ia32_reduce_fmul_ps512:
   case X86::BI__builtin_ia32_reduce_fmul_ph512:
   case X86::BI__builtin_ia32_reduce_fmul_ph256:
   case X86::BI__builtin_ia32_reduce_fmul_ph128: {
     Function *F =
         CGM.getIntrinsic(Intrinsic::vector_reduce_fmul, Ops[1]->getType());

clang/test/CodeGen/builtins-x86-reduce.c

+// RUN: %clang_cc1 %s -triple x86_64-unknown-unknown -target-feature +avx512f -emit-llvm -o - | FileCheck %s
+
+typedef double double8 __attribute__((ext_vector_type(8)));
+
+double foo(double8 a, double b) {
+  return __builtin_ia32_reduce_fmax_pd512(a) + b;
+}
+
+// CHECK: fadd
+// CHECK-NOT: nnan
+// CHECK-SAME: double