Make lround builtin constexpr (and others)

[zoecarver]

Summary

This patch makes some of the lround and lrint builtins constant expressions. Resolves issue 38322.

I can update more builtins but, first wanted to make sure this was the correct way of implementing the constexpr builtins.

Feel free to add other reviewers. I wasn't really sure who to add.

[erichkeane]

Small nit, just default construct the APFloats.

[craig.topper]

What if the input is nan or infinity or out of range. The spec says an implementation defined value is returned. But we've deferred to whatever library we're compiled with. Which makes our implementation defined behavior outside of clang's control and it now depends on how clang is built.

[erichkeane]

Craig topper pointed out to me that constexpr cannot throw exceptions, and at least the rints can raise some exceptions, which aren't allowed in Constexpr. The round functions similarly hit unspecified behavior, so we need to match that behavior.

[lebedev.ri]

clang/test/SemaCXX/math-builtins.cpp

5

Needs more tests
This looks like you always round up

[zoecarver]

(from IRC) Add tests for:

• out of range
• nan
• rounding up _and_ down

Also, use the builtin APFloat methods.

[rsmith]

clang/lib/AST/ExprConstant.cpp

9612

It's non-conforming to accept calls to these non-__builtin functions in constant expression evaluation, but it's fine to constant-fold them. Please issue a CCEDiag diagnostic for them (take a look at what we do for Builtin::BIstrlen or Builtin::BIstrcmp for an example; it would probably be reasonable to factor out a local lambda to issue suitable diagnostics for this case).

(For example, we are required to produce a diagnostic for constexpr long n = lround(1.2);, but we are permitted to constant-fold long n = lround(1.2); and emit it with static initialization.)

9616

This assumes that the host double has the same behavior as the target double, which is not true in general. Generally the right thing to do is to implement the relevant functionality on APFloat, taking care to produce the right answer for the floating-point model being used by that APFloat. This also assumes that the host long is the same size as the target long, which again is not true in general. Finally, you need to catch the case where the APFloat does not fit into the target type; in that case, the best response is probably to treat the evaluation as non-constant and leave it to the runtime library to implement whatever error response it chooses.

[zoecarver]

• remove rint updates (rint uses runtime-defined rounding method)
• add more tests (nan, overflow, round down)
• use APFloat rounding methods
• if rounding fails, bail out to runtime

[zoecarver]

clang/lib/AST/ExprConstant.cpp

9612

Thank you for pointing this out. I will do that in a separate patch.

9616

I've updated it to do just that: if there is an error, it will treat this evaluation as non-constant and leave it for the runtime library to handle.

How should I determine the width of the APInt? Because it is run at compile-time, can I use the max size (long long)? Either way, how should I get the correct size, maybe from a builtin long type?

clang/test/SemaCXX/math-builtins.cpp

21

For some reasons, these expected-errors aren't being caught by lit. Any ideas? Do I need to put it in a .fail.cpp file or add something to the RUN: comment?

[lebedev.ri]

clang/test/SemaCXX/math-builtins.cpp

2

-verify ?

[lebedev.ri]

clang/lib/AST/ExprConstant.cpp

9616

You probably want to look at the original callexpr, take it's return type, and look it up in typesystem (i'm not sure exactly how to do that here)

[zoecarver]

clang/lib/AST/ExprConstant.cpp

9616

Ah, good idea. Will do.

clang/test/SemaCXX/math-builtins.cpp

2

Yes, that worked. It was throwing me off the first time I tried because it doesn't expect the nan tests to error. Any ideas on generating a nan float? Thanks for the help :)

[craig.topper]

clang/lib/AST/ExprConstant.cpp

9616

Info.Ctx.getIntWidth(E->getType()) I think.

[zoecarver]

clang/lib/AST/ExprConstant.cpp

9616

@craig.topper fantastic, you're the best!

[craig.topper]

clang/lib/AST/ExprConstant.cpp

9620

Space after if

9623

Space after if

[zoecarver]

• fix expected error
• fix APInt width
• fix nan tests

[zoecarver]

• fix: formatting

[rsmith]

clang/lib/AST/ExprConstant.cpp

9617

Please use /*isUnsigned=*/false for the second argument rather than 0.

9618

Please capitalize local variable names to match the convention used in this file.

9625

Just return Success(IVal, E); here. Going via getExtValue will assert if the integer type is wider than 64 bits.

[efriedma]

It's probably worth adding testcases for 0.5 and -0.5. I think the current implementation behaves correctly, but it would be easy to mess up with a small change to the code.

Are you intentionally excluding __builtin_lroundl/__builtin_llroundl?

clang/lib/AST/ExprConstant.cpp

9625

getExtVal call here is unnecessary.

[zoecarver]

clang/lib/AST/ExprConstant.cpp

9617

I'm using the default value of isUnsigned (false). Do you want me to specify it? The second argument (0) is the initial value (which is required).

[zoecarver]

Are you intentionally excluding builtin_lroundl/builtin_llroundl?

I was because convertToDouble could only return up to 64 bytes. But now that I am using the builtin APFloat round function, that works.

[zoecarver]

• add roundl builtins
• add more tests
• address review comments

[zoecarver]

Ping. Anything else I am missing?

clang/lib/AST/ExprConstant.cpp

case Builtin::BI__builtin_ffsll: {
     APSInt Val;
     if (!EvaluateInteger(E->getArg(0), Val, Info))
       return false;
 
     unsigned N = Val.countTrailingZeros();
     return Success(N == Val.getBitWidth() ? 0 : N + 1, E);
   }
 
+  case Builtin::BIlround:
+  case Builtin::BI__builtin_lround: {
+    APFloat Val(0.0);
+    return EvaluateFloat(E->getArg(0), Val, Info) &&
+           Success(lround(Val.convertToDouble()), E);
+  }
+  case Builtin::BIlroundf:
+  case Builtin::BI__builtin_lroundf: {
+    APFloat Val(0.0);
+    return EvaluateFloat(E->getArg(0), Val, Info) &&
+           Success(lround(Val.convertToFloat()), E);
+  }
+
+  case Builtin::BIllround:
+  case Builtin::BI__builtin_llround: {
+    APFloat Val(0.0);
+    return EvaluateFloat(E->getArg(0), Val, Info) &&
+           Success(lround(Val.convertToDouble()), E);
+  }
+  case Builtin::BIllroundf:
+  case Builtin::BI__builtin_llroundf: {
+    APFloat Val(0.0);
+    return EvaluateFloat(E->getArg(0), Val, Info) &&
+           Success(llround(Val.convertToFloat()), E);
+  }
+
+  case Builtin::BIlrint:
+  case Builtin::BI__builtin_lrint: {
+    APFloat Val(0.0);
+    return EvaluateFloat(E->getArg(0), Val, Info) &&
+           Success(lround(Val.convertToDouble()), E);
+  }
+  case Builtin::BIlrintf:
+  case Builtin::BI__builtin_lrintf: {
+    APFloat Val(0.0);
+    return EvaluateFloat(E->getArg(0), Val, Info) &&
+           Success(lrint(Val.convertToFloat()), E);
+  }
+
+  case Builtin::BIllrint:
+  case Builtin::BI__builtin_llrint: {
+    APFloat Val(0.0);
+    return EvaluateFloat(E->getArg(0), Val, Info) &&
+           Success(lround(Val.convertToDouble()), E);
+  }
+  case Builtin::BIllrintf:
+  case Builtin::BI__builtin_llrintf: {
+    APFloat Val(0.0);
+    return EvaluateFloat(E->getArg(0), Val, Info) &&
+           Success(llrint(Val.convertToFloat()), E);
+  }
+
   case Builtin::BI__builtin_fpclassify: {
     APFloat Val(0.0);
     if (!EvaluateFloat(E->getArg(5), Val, Info))
       return false;
     unsigned Arg;
     switch (Val.getCategory()) {
     case APFloat::fcNaN: Arg = 0; break;
     case APFloat::fcInfinity: Arg = 1; break;

clang/test/SemaCXX/math-builtins.cpp

+// RUN: %clang_cc1 -std=c++11 %s
+
+int main()
+{
+  constexpr float f = 12345.6789;
+
+  static_assert(__builtin_llround(f) == 12346, "");
+  static_assert(__builtin_llroundf(f) == 12346, "");
+
+  static_assert(__builtin_llrint(f) == 12346, "");
+  static_assert(__builtin_llrintf(f) == 12346, "");
+
+  static_assert(__builtin_lrint(f) == 12346, "");
+  static_assert(__builtin_lrintf(f) == 12346, "");
+
+  static_assert(__builtin_lround(f) == 12346, "");
+  static_assert(__builtin_lroundf(f) == 12346, "");
+}
 No newline at end of file