[PATCH] D129231: [Builtins] Do not claim all libfuncs are readnone with trapping math.

Thu Aug 11 04:31:05 PDT 2022

fhahn added inline comments.

================
Comment at: clang/include/clang/Basic/Builtins.def:1409

-LIBBUILTIN(round, "dd", "fnc", "math.h", ALL_LANGUAGES)
-LIBBUILTIN(roundf, "ff", "fnc", "math.h", ALL_LANGUAGES)
-LIBBUILTIN(roundl, "LdLd", "fnc", "math.h", ALL_LANGUAGES)
+LIBBUILTIN(round, "dd", "fng", "math.h", ALL_LANGUAGES)
+LIBBUILTIN(roundf, "ff", "fng", "math.h", ALL_LANGUAGES)
----------------
john.brawn wrote:
> scanon wrote:
> > `round` is just like `trunc` or `floor` or `ceil`, and should be "fnc" if they are.
> I think they all should be the same, but I'm note sure if they should all be "fnc" or "fng".
> 
> Looking at the C17 standard, in section F.10.6 (IEE754/IEC60559 behaviour of nearest integer functions) round, trunc, floor, and ceil are all described in the same way: "The xyz functions may, but are not required to, raise the "inexact" floating-point exception for finite non-integer arguments". I don't know if our policy should be "These functions may raise an exception, therefore we should assume they do and treat that as a side-effect", or "These functions are not required to raise an exception, therefore a program can't rely on it and so we don't need to treat it as a side-effect".
> 
> Looking at implementations of these functions, it looks like GNU libm doesn't raise inexact, but the bionic libm does. I think I'm leaning towards marking all of them as "fng" as it's the more cautious of the two.
> Looking at implementations of these functions, it looks like GNU libm doesn't raise inexact, but the bionic libm does. I think I'm leaning towards marking all of them as "fng" as it's the more cautious of the two.

Hmm, bionic's behavior sounds a bit surprising. In the committed version, I kept them `fnc` to keep the previous behavior for now. I'll double-check with @scanon and potentially upload a follow-up patch.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D129231