[libcxx-commits] [PATCH] D70631: Microsoft's floating-point to_chars powered by Ryu and Ryu Printf

[Martin Storsjö via Phabricator via libcxx-commits]

mstorsjo added inline comments.


================
Comment at: libcxx/src/include/ryu/d2s_intrinsics.h:49
+
+#if defined(_M_X64) && defined(_LIBCPP_COMPILER_MSVC)
+#define _LIBCPP_INTRINSIC128 1
----------------
I think you could loosen this into `#if defined(_M_X64) && defined(_MSC_VER)`; `_LIBCPP_COMPILER_MSVC` is only defined when building with the actual MSVC, not clang-cl impersonating MSVC. (And the real MSVC isn't supported for building libcxx in practice.)

Or did this codepath give errors when built with clang-cl before?


================
Comment at: libcxx/src/include/ryu/d2s_intrinsics.h:71
+    (defined(__GNUC__) && !defined(__clang__) && !defined(__CUDACC__)) || \
+    (defined(_M_X64) && !defined(__MINGW32__) && !defined(_LIBCPP_COMPILER_MSVC)))
+#define _LIBCPP_INTRINSIC128 1
----------------
Isn't this condition kinda opposite to what you suggest in the comment? I.e. the comment writes (and we discussed) that mingw does support int128, but then you check `defined(_M_X64) && !defined(__MINGW32__)` - I believe this codepath isn't used here as intended.

Actually, I would suggest simplifying this whole condition into this:

```
#elif defined(__SIZEOF_INT128__) && ( \
    (defined(__clang__) && !defined(_MSC_VER)) || \
    (defined(__GNUC__) && !defined(__clang__) && !defined(__CUDACC__)))
```

Thus, if we have `__SIZEOF_INT128__` defined (this should exclude all 32 bit targets, so we don't need to check for any 64 bit architecture specifically). This check in itself is almost enough as such. But then as a extra requirement, we'd only try this if we also have specifically clang or gcc. And for clang, we'd exclude building in MSVC mode, where we might not have all the runtime functions available. (If it turns out that the code below doesn't actually generate any calls to runtime helpers, we wouldn't need the check for `_MSC_VER` at all.)

Or it could even be as simple as this:
```
#elif !defined(_LIBCPP_HAS_NO_INT128) && !defined(_MSC_VER)
```

We already have `_LIBCPP_HAS_NO_INT128` in libcxx, and it might be good to honor it. However that define is only based on compiler support - in MSVC mode, it doesn't know that we actually don't have all the necessary runtime functions.

> We have __uint128 support in clang or gcc or MinGW 64

Those three aren't really three comparable things - when building for mingw, you'd be using either clang or gcc. Maybe "We have __uint128 support in gcc and clang, except when in MSVC mode" (or clang-cl instead of MSVC)?



Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70631/new/

https://reviews.llvm.org/D70631