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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
