Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Is there a specific reason to not constexprify Dragonbox? #2506

Closed
jk-jeon opened this issue Sep 20, 2021 · 3 comments
Closed

Is there a specific reason to not constexprify Dragonbox? #2506

jk-jeon opened this issue Sep 20, 2021 · 3 comments

Comments

@jk-jeon
Copy link
Contributor

jk-jeon commented Sep 20, 2021

Is there a specific reason to not constexprify Dragonbox? If not, I believe it should be not terribly difficult to do so, because as far as I know the only real obstacle is that some intrinsics are not constexpr. But we can just wrap those intrinsics with if (!is_constant_evaluated()) and provide fallback implementations along with that. I'm experimenting myself with the recent master branch, and I guess Dragonbox is fine while it fails at this line, I do not know why.

Another issue is that the compiler is whining about static constexpr tables inside constexpr functions being static. (See e.g., https://stackoverflow.com/questions/62458079/static-constexpr-variables-in-a-constexpr-function.) Honestly I think this is a sort of nonsensical language restriction, but well, it is like that. A simple solution is to move those tables out of the functions.

@alexezeder
Copy link
Contributor

Since I was responsible for the recently implemented constexprification of FP formatting, I guess I can try to answer that.

Is there a specific reason to not constexprify Dragonbox?

I don't have a specific reason for this, it was just mentioned by @vitaut that the most feasible approach is to use Dragon4 at compile-time. I agreed with this because Dragonbox FP formatting code seems complicated for me, thus it could have some ideas that cannot be constexprified easily. Also, this algorithm is used only for numbers without additional formatting specs, if I'm correct. Besides, who knows, maybe Dragonbox wouldn't be fast at compile-time the same way it's fast at runtime, this should be benchmarked, of course.

If not, I believe it should be not terribly difficult to do so

Sure, but it requires some work, additional work considering choosing the most feasible approach. I'm open to making Dragonbox available at compile-time 😉, but I don't think it would help {fmt} somehow.

Another issue is that the compiler is whining about static constexpr tables inside constexpr functions being static

Yeah, it's a familiar problem... probably for everyone who tried to make their code constexpr. 🤦

@jk-jeon
Copy link
Contributor Author

jk-jeon commented Sep 22, 2021

As I mentioned, I was already able to make Dragonbox constexpr, but since it is relying on large static tables it probably works only with FMT_HEADER_ONLY. For whatever reason I thought it was Grisu instead of Dragon4 that is being used right now, so I thought this restriction is still there with the current implementation. Now I get that it's not the case, and it seems very reasonable to relying on Dragon4 instead.

@vitaut You can just close this issue, if you don't have further comments.

Actually, I thought that compilers are able to optimize this code (based on the fact that this approach is already used in the library - power_of_10_64 or bsr2log10), but I was wrong making this assumption: https://godbolt.org/z/KWYhcqKxn (Clang ❤️).

BTW, yeah, I was also very disappointed with this. Probably because adding static data that is not explicitly asked by the programmer is something compilers are not recommended to do? I don't know. Sounds silly to me to be honest, just like the fact that static is forbidden in constexpr context...

@vitaut
Copy link
Contributor

vitaut commented Sep 24, 2021

I don't have much to add to what @alexezeder wrote. I think Dragon4 is OK for now but if it proves to be slow in terms of compile time affecting real use cases we could look into ways to optimize it in the future, possibly by constexprifying Dragonbox.

@vitaut vitaut closed this as completed Sep 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants