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

Wed Dec 21 07:06:41 PST 2022

arsenm added a comment.

In D140467#4010675 <https://reviews.llvm.org/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

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D140467/new/

https://reviews.llvm.org/D140467