[PATCH] D140467: [X86][Reduce] Preserve fast math flags when change it. NFCI
Roman Lebedev via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Wed Dec 21 07:02:12 PST 2022
lebedev.ri requested changes to this revision.
lebedev.ri added a comment.
In D140467#4010675 <https://reviews.llvm.org/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.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D140467/new/
https://reviews.llvm.org/D140467
More information about the cfe-commits
mailing list