Page MenuHomePhabricator

[CodeGen] fix const-ness of builtin equivalents of <math.h> and <complex.h> functions that might set errno
ClosedPublic

Authored by spatel on Oct 31 2017, 3:21 PM.

Details

Summary

As the description in Builtins.def says: "e = const, but only when -fmath-errno=0".

This is step 2 of 3 to fix builtins and math calls as discussed in D39204.

I went through the lists at:
http://en.cppreference.com/w/c/numeric/math
http://en.cppreference.com/w/c/numeric/complex
...and changed all of the builtins that correspond to functions that could set errno.

The math.h functions that are specified to never set errno are:
fmax
fmin
nearbyint

And for complex.h:
creal
cimag
conj
cproj

powi is not a standard libm function, so that's also left as "Fnc".

Diff Detail

Repository
rL LLVM

Event Timeline

spatel created this revision.Oct 31 2017, 3:21 PM
efriedma edited edge metadata.Oct 31 2017, 4:16 PM

Please make these consistent with the non-__builtin versions.

Granted, I'm not sure all of those are right... I'm pretty sure, for example, cbrt can't set errno, and carg can. But I'd prefer to deal with that in a separate patch.

spatel added a comment.Nov 1 2017, 7:24 AM

Granted, I'm not sure all of those are right... I'm pretty sure, for example, cbrt can't set errno, and carg can. But I'd prefer to deal with that in a separate patch.

I figured we have to be ultra-conservative here, and there are docs out there like this:

"On successful completion, cbrt() returns the cube root of x. If x is NaN, cbrt() returns NaN and errno may be set to [EDOM]. "

http://pubs.opengroup.org/onlinepubs/7908799/xsh/cbrt.html

I think we should assume libm complies with the C99 standard (specifically, the appendix specifying the behavior of IEEE floating-point). Otherwise, we have no basis to predict the behavior of any standard library call.

From the standard: "Functions with a NaN argument return a NaN result and raise no floating-point exception, except where stated otherwise."

There's an oddity with fma. The version without __builtin has 'e' already

LIBBUILTIN(fma, "dddd", "fne", "math.h", ALL_LANGUAGES)
LIBBUILTIN(fmaf, "ffff", "fne", "math.h", ALL_LANGUAGES)
LIBBUILTIN(fmal, "LdLdLdLd", "fne", "math.h", ALL_LANGUAGES)

But we don't check the const attribute in CGBuiltin.cpp before converting it to an intrinsic

case Builtin::BIfma:
case Builtin::BIfmaf:
case Builtin::BIfmal:
case Builtin::BI__builtin_fma:
case Builtin::BI__builtin_fmaf:
case Builtin::BI__builtin_fmal: {
  // Rewrite fma to intrinsic.
  Value *FirstArg = EmitScalarExpr(E->getArg(0));
  llvm::Type *ArgType = FirstArg->getType();
  Value *F = CGM.getIntrinsic(Intrinsic::fma, ArgType);
  return RValue::get(
      Builder.CreateCall(F, {FirstArg, EmitScalarExpr(E->getArg(1)),
                             EmitScalarExpr(E->getArg(2))}));
}

But we don't check the const attribute in CGBuiltin.cpp before converting it to an intrinsic

In practice, fma() implementations don't set errno (at least, glibc and msvcrt don't). And people would be really annoyed if we forced them to use fast-math flags to generate an fma instruction. So this is probably okay?

hfinkel edited edge metadata.Nov 2 2017, 11:10 AM

There's an oddity with fma. The version without __builtin has 'e' already

Something that is potentially relevant: POSIX says that fma() can set errno (http://pubs.opengroup.org/onlinepubs/9699919799/functions/fma.html), however, the glibc man page says that its fma() does not set errno (https://linux.die.net/man/3/fma).

LIBBUILTIN(fma, "dddd", "fne", "math.h", ALL_LANGUAGES)
LIBBUILTIN(fmaf, "ffff", "fne", "math.h", ALL_LANGUAGES)
LIBBUILTIN(fmal, "LdLdLdLd", "fne", "math.h", ALL_LANGUAGES)

But we don't check the const attribute in CGBuiltin.cpp before converting it to an intrinsic

case Builtin::BIfma:
case Builtin::BIfmaf:
case Builtin::BIfmal:
case Builtin::BI__builtin_fma:
case Builtin::BI__builtin_fmaf:
case Builtin::BI__builtin_fmal: {
  // Rewrite fma to intrinsic.
  Value *FirstArg = EmitScalarExpr(E->getArg(0));
  llvm::Type *ArgType = FirstArg->getType();
  Value *F = CGM.getIntrinsic(Intrinsic::fma, ArgType);
  return RValue::get(
      Builder.CreateCall(F, {FirstArg, EmitScalarExpr(E->getArg(1)),
                             EmitScalarExpr(E->getArg(2))}));
}
spatel updated this revision to Diff 121344.Nov 2 2017, 12:02 PM

Patch updated:
Make const-ness of the builtins match const-ness of their lib function siblings. We're deferring fixing some of these that are obviously wrong to follow-up patches. Hopefully, the bugs are visible in the new test file (added at rL317220).

These are the differences I noted from the previous rev of the patch:
ceil, floor, rint, round, and everything in complex are left as constant regardless of errno.
lgamma is not constant regardless of errno.

efriedma accepted this revision.Nov 2 2017, 1:18 PM

LGTM.

(For reference, lgamma is weird because of the POSIX signgam.)

This revision is now accepted and ready to land.Nov 2 2017, 1:18 PM
This revision was automatically updated to reflect the committed changes.