Izaron added inline comments.
================ Comment at: clang/lib/AST/ExprConstant.cpp:14033-14034 + !EvaluateFloat(E->getArg(1), RHS, Info)) + return false; + if (Result.isNaN() || RHS > Result) + Result = RHS; ---------------- jcranmer-intel wrote: > If I'm reading APFloat's source correctly, this doesn't suffice to > consistently make __builtin_fmax(±0.0, ±0.0) return +0.0 if one of the zeroes > is positive. (We're not required to return +0.0 in this case, but most libm > implementations seem to endeavor to return +0.0 in this situation, even if > the LLVM intrinsic we lower to doesn't). These cases are covered in tests (in a wrong manner though as I see), I was "checking" that we have `+0.0` if one of the zeroes is positive ================ Comment at: clang/test/Sema/constant-builtins-fmax.cpp:35-39 +#define FMAX_TEST_BOTH_ZERO(T, FUNC) \ + static_assert(0.0 == FUNC(0.0, 0.0)); \ + static_assert(0.0 == FUNC(-0.0, 0.0)); \ + static_assert(0.0 == FUNC(0.0, -0.0)); \ + static_assert(0.0 == FUNC(-0.0, -0.0)); ---------------- jcranmer-intel wrote: > These tests aren't covering what the sign of zero is in these cases. Cool! Thank you, today I learned that this compiles: ``` static_assert(0.0 == 0.0); static_assert(0.0 == -0.0); static_assert(0.0 == +0.0); ``` So I think I need to test the `+0.0` cases with the `__builtin_signbit` (I didn't find another way), BUT this function is not constexpr yet. So I need to implement a constexpr `__builtin_signbit` first... In a new pull request. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134369/new/ https://reviews.llvm.org/D134369 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
