@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.
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| clang/lib/CodeGen/CGBuiltin.cpp | ||
|---|---|---|
| 14746 | 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. | |
| clang/lib/CodeGen/CGBuiltin.cpp | ||
|---|---|---|
| 14746 | No, it couldn't be, just tried :( | |
| clang/lib/CodeGen/CGBuiltin.cpp | ||
|---|---|---|
| 14744 | We have FastMathFlagGuard for automatically saving and restoring fast math flags. | |
Comment Actions
Use FastMathFlagGuard instead, thanks @foad!
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 | ||
|---|---|---|
| 14746 | Thanks for trying it :) | |
Comment Actions
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.
Comment Actions
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
Comment Actions
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/.
Comment Actions
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
Comment Actions
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.
Comment Actions
Testing is always feasible. You could even just generate all the combinations
| clang/lib/CodeGen/CGBuiltin.cpp | ||
|---|---|---|
| 13120 | Should only do this in the cases that actually set the flags | |
| clang/test/CodeGen/builtins-x86-reduce.c | ||
| 9 | Should test the builtins from both sets | |
| 10–12 | use update_cc_test_checks, check-not is super fragile | |
Comment Actions
Address review comments. Thanks!
| clang/test/CodeGen/builtins-x86-reduce.c | ||
|---|---|---|
| 9 | Do you mean this? | |
| clang/test/CodeGen/builtins-x86-reduce.c | ||
|---|---|---|
| 9 | 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 | |
Should only do this in the cases that actually set the flags