[PATCH] D66862: Make lround builtin constexpr (and others)

zoecarver added a comment. Ping. Anything else I am missing? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66862/new/ https://reviews.llvm.org/D66862 ___ cfe-commits mailing list cfe-commits@lists.llvm

[PATCH] D66862: Make lround builtin constexpr (and others)

zoecarver updated this revision to Diff 218231. zoecarver added a comment. - add roundl builtins - add more tests - address review comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66862/new/ https://reviews.llvm.org/D66862 Files: clang/lib

[PATCH] D66862: Make lround builtin constexpr (and others)

zoecarver marked an inline comment as done. zoecarver added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:9617 +APFloat FPVal(0.0); +APSInt IVal(Info.Ctx.getIntWidth(E->getType()), 0); +bool isExact = true; rsmith wrote: > Please use `/*

[PATCH] D66862: Make lround builtin constexpr (and others)

zoecarver added a comment. > 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. Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D66862: Make lround builtin constexpr (and others)

efriedma added a comment. 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`?

[PATCH] D66862: Make lround builtin constexpr (and others)

rsmith added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:9617 +APFloat FPVal(0.0); +APSInt IVal(Info.Ctx.getIntWidth(E->getType()), 0); +bool isExact = true; Please use `/*isUnsigned=*/false` for the second argument rather than `0`.

[PATCH] D66862: Make lround builtin constexpr (and others)

craig.topper added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:9620 + +if(!EvaluateFloat(E->getArg(0), FPVal, Info)) return false; +APFloat::opStatus status = Space after if Comment at: clang/lib/AST/ExprConstant.cpp:962

[PATCH] D66862: Make lround builtin constexpr (and others)

zoecarver updated this revision to Diff 217976. zoecarver added a comment. - fix expected error - fix APInt width - fix nan tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66862/new/ https://reviews.llvm.org/D66862 Files: clang/lib/AST/ExprC

[PATCH] D66862: Make lround builtin constexpr (and others)

zoecarver updated this revision to Diff 217977. zoecarver added a comment. - fix: formatting Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66862/new/ https://reviews.llvm.org/D66862 Files: clang/lib/AST/ExprConstant.cpp clang/test/SemaCXX/math

[PATCH] D66862: Make lround builtin constexpr (and others)

zoecarver marked an inline comment as done. zoecarver added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:9616 +return EvaluateFloat(E->getArg(0), Val, Info) && + Success(lround(Val.convertToDouble()), E); + } craig.topper wrote: > zo

[PATCH] D66862: Make lround builtin constexpr (and others)

craig.topper added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:9616 +return EvaluateFloat(E->getArg(0), Val, Info) && + Success(lround(Val.convertToDouble()), E); + } zoecarver wrote: > lebedev.ri wrote: > > zoecarver wrote: > > > r

[PATCH] D66862: Make lround builtin constexpr (and others)

lebedev.ri added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:9616 +return EvaluateFloat(E->getArg(0), Val, Info) && + Success(lround(Val.convertToDouble()), E); + } zoecarver wrote: > rsmith wrote: > > This assumes that the host `do

[PATCH] D66862: Make lround builtin constexpr (and others)

zoecarver marked 2 inline comments as done. zoecarver added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:9616 +return EvaluateFloat(E->getArg(0), Val, Info) && + Success(lround(Val.convertToDouble()), E); + } lebedev.ri wrote: > zoec

[PATCH] D66862: Make lround builtin constexpr (and others)

lebedev.ri added inline comments. Comment at: clang/test/SemaCXX/math-builtins.cpp:1 +// RUN: %clang_cc1 -std=c++11 %s + -verify ? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66862/new/ https://reviews.llvm.org/

[PATCH] D66862: Make lround builtin constexpr (and others)

zoecarver marked 3 inline comments as done. zoecarver added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:9612 + case Builtin::BIlround: + case Builtin::BI__builtin_lround: { rsmith wrote: > It's non-conforming to accept calls to these non-`__bu

[PATCH] D66862: Make lround builtin constexpr (and others)

zoecarver updated this revision to Diff 217973. zoecarver added a comment. - 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 Repository: rG LLVM Github Monorepo

[PATCH] D66862: Make lround builtin constexpr (and others)

rsmith requested changes to this revision. rsmith added inline comments. Comment at: clang/lib/AST/ExprConstant.cpp:9612 + case Builtin::BIlround: + case Builtin::BI__builtin_lround: { It's non-conforming to accept calls to these non-`__builtin` functions in

[PATCH] D66862: Make lround builtin constexpr (and others)

zoecarver added a comment. (from IRC) Add tests for: - out of range - nan - rounding up _and_ down Also, use the builtin APFloat methods. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66862/new/ https://reviews.llvm.org/D66862

[PATCH] D66862: Make lround builtin constexpr (and others)

erichkeane requested changes to this revision. erichkeane added a comment. This revision now requires changes to proceed. 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 functio

[PATCH] D66862: Make lround builtin constexpr (and others)

craig.topper added a comment. 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 depend

[PATCH] D66862: Make lround builtin constexpr (and others)

lebedev.ri added inline comments. Comment at: clang/test/SemaCXX/math-builtins.cpp:5 +{ + constexpr float f = 12345.6789; + Needs more tests This looks like you always round up Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.

[PATCH] D66862: Make lround builtin constexpr (and others)

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. Small nit, just default construct the APFloats. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66862/new/ https://reviews.llvm.org/D66862

[PATCH] D66862: Make lround builtin constexpr (and others)

zoecarver created this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D66862 Files: clang/lib/AST/ExprConstant.cpp clang/test/SemaCXX/math-builtins.cpp Index: clang/test/SemaCXX/math-builtins.c