This is an archive of the discontinued LLVM Phabricator instance.

[Builtins] Do not claim all libfuncs are readnone with trapping math.
ClosedPublic

Authored by fhahn on Jul 6 2022, 3:05 PM.

Details

Summary

At the moment, Clang only considers errno when deciding if a builtin
is const. This ignores the fact that some library functions may raise
floating point exceptions, which may modify global state, e.g. when
updating FP status registers.

To model the fact that some library functions/builtins may raise
floating point exceptions, this patch adds a new 'g' modifier for
builtins. If a builtin is marked with 'g', it cannot be considered
const, unless FP exceptions are ignored.

So far I've not added CHECK lines for all calls in math-libcalls.c. I'll
do that once we agree on the overall direction.

A consequence seems to be that we fail to select some of the constrained
math builtins now, but I am not entirely sure what's going on there.

Diff Detail

Event Timeline

fhahn created this revision.Jul 6 2022, 3:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 6 2022, 3:05 PM
fhahn requested review of this revision.Jul 6 2022, 3:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 6 2022, 3:05 PM

Looking at the descriptions of maths functions in C99 (and I expect C11 will be the same) it looks like there are three kinds:

  • Those that can report error by errno and floating-point exeption, and may also raise the inexact exception
  • Those that don't set errno, but may raise the inexact exception
  • Those that neither set errno or raise an exception

Looking at this patch and the attributes of the various function intrinsics it looks like you have:

  • Marked "e": Can set errno, can't raise exception
  • Marked "eg": Can set errno and raise exception
  • Marked "cg": Can't set errno, can raise an exception
  • Marked "c": Can't set errno or raise an exception

Given that the functions that set errno also raise exceptions I think it would make sense to have the "e" attribute cover both errno and exceptions, and have "g" just for those that can only raise exceptions. Also "cg" looks like it's probably wrong and should be "g" (given that "c" means "always const" and "g" means "const only when we don't have exceptions", and that's also how "e" is used in that we don't have functions with "cg").

fhahn updated this revision to Diff 448628.Jul 29 2022, 7:40 AM

Looking at the descriptions of maths functions in C99 (and I expect C11 will be the same) it looks like there are three kinds:

  • Those that can report error by errno and floating-point exeption, and may also raise the inexact exception
  • Those that don't set errno, but may raise the inexact exception
  • Those that neither set errno or raise an exception

Looking at this patch and the attributes of the various function intrinsics it looks like you have:

  • Marked "e": Can set errno, can't raise exception
  • Marked "eg": Can set errno and raise exception
  • Marked "cg": Can't set errno, can raise an exception
  • Marked "c": Can't set errno or raise an exception

Given that the functions that set errno also raise exceptions I think it would make sense to have the "e" attribute cover both errno and exceptions, and have "g" just for those that can only raise exceptions. Also "cg" looks like it's probably wrong and should be "g" (given that "c" means "always const" and "g" means "const only when we don't have exceptions", and that's also how "e" is used in that we don't have functions with "cg").

Thanks you very much for taking a look! I updated the patch to redefine "e" to mean const if no errno and no exceptions. Now the only markings are either "e" or "g". The functions originally marked "cg" were indeed a mistake.

This revision is now accepted and ready to land.Aug 5 2022, 9:49 AM
scanon added inline comments.Aug 5 2022, 1:12 PM
clang/include/clang/Basic/Builtins.def
1409

round is just like trunc or floor or ceil, and should be "fnc" if they are.

john.brawn added inline comments.Aug 8 2022, 3:57 AM
clang/include/clang/Basic/Builtins.def
1409

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.

scanon added a comment.Aug 8 2022, 8:56 AM

The CFP working group and C23 have since clarified this in Annex F:

The returned value is exact and is independent of the current rounding direction mode.

They never set inexact on an implementation that claims 60559 conformance. The only flag that these operations may set is invalid on signaling NaN input.

fhahn added inline comments.Aug 11 2022, 4:31 AM
clang/include/clang/Basic/Builtins.def
1409

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.

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.

As noted, this is only clarified in C23. Previous C standards did not specify whether or not these functions should set inexact, leading to some implementations choosing one behavior and others choosing the other. That said, the TS that clarified the behavior has been available for about a decade, so I'm mildly surprised that Bionic kept their existing behavior for so long.

In any event, from the standpoint of the C(23) language, these operations do not set inexact, so I believe that it is appropriate to optimize them as if they do not set inexact.

fhahn added a comment.Aug 11 2022, 9:40 AM

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.

As noted, this is only clarified in C23. Previous C standards did not specify whether or not these functions should set inexact, leading to some implementations choosing one behavior and others choosing the other. That said, the TS that clarified the behavior has been available for about a decade, so I'm mildly surprised that Bionic kept their existing behavior for so long.

Hm, I think making this depend on the C standard version would be rather tricky, as I don't think there's precedence for that at the moment. For ceil/round it won't matter in practice, because we will lower them to the constrained intrinsics.

fhahn added a comment.Aug 16 2022, 1:45 AM

@john.brawn Are you OK with treating those functions as not setting inexact, as per the C23 clarification, as the committed version does?