Skip to content

[libc][msvc] fix mathlib build on WoA #161258

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Sep 29, 2025
Merged

[libc][msvc] fix mathlib build on WoA #161258

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
582d712
[libc][msvc] fix mathlib build on WoA
SchrodingerZhu Sep 29, 2025
a5477ff
fix
SchrodingerZhu Sep 29, 2025
2de3ad1
fix typo
SchrodingerZhu Sep 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions libc/src/string/memory_utils/op_generic.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,12 +41,22 @@ static_assert((UINTPTR_MAX == 4294967295U) ||
"We currently only support 32- or 64-bit platforms");

#ifdef LIBC_COMPILER_IS_MSVC

#ifdef LIBC_TARGET_ARCH_IS_X86
namespace LIBC_NAMESPACE_DECL {
using generic_v128 = __m128i;
using generic_v256 = __m256i;
using generic_v512 = __m512i;
Copy link
Contributor Author

@SchrodingerZhu SchrodingerZhu Sep 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jhuber6 enforcing the existence of generic_v512 looks suspicious. Do you know if this is really required?

} // namespace LIBC_NAMESPACE_DECL
#else
// Special handling when target does not have real vector types.
// We can potentially use uint8x16_t etc. However, MSVC does not provide
// subscript operation.
namespace LIBC_NAMESPACE_DECL {
Copy link

Copilot AI Sep 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The placeholder vector type structs lack documentation explaining their purpose as MSVC-specific workarounds. Add comments explaining why these are needed and when they're used instead of native vector types.

Suggested change
namespace LIBC_NAMESPACE_DECL {
namespace LIBC_NAMESPACE_DECL {
// MSVC-specific workaround:
// On non-x86 targets, MSVC does not provide native vector types or subscriptable vector types
// (such as uint8x16_t). As a workaround, we define these placeholder structs as aligned arrays
// of uint8_t. These are used in place of native vector types when compiling with MSVC on
// targets that lack real vector support.

Copilot uses AI. Check for mistakes.
struct alignas(16) generic_v128 : public cpp::array<uint8_t, 16> {};
struct alignas(32) generic_v256 : public cpp::array<uint8_t, 32> {};
struct alignas(64) generic_v512 : public cpp::array<uint8_t, 64> {};
} // namespace LIBC_NAMESPACE_DECL
#endif

#else
namespace LIBC_NAMESPACE_DECL {
Expand Down Expand Up @@ -159,7 +169,8 @@ template <typename T> struct Memset {

LIBC_INLINE static void block(Ptr dst, uint8_t value) {
if constexpr (is_scalar_v<T> || is_vector_v<T>) {
store<T>(dst, splat<T>(value));
// Avoid ambiguous call due to ADL
Copy link

Copilot AI Sep 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment mentions ADL (Argument-Dependent Lookup) but doesn't explain what specific ambiguity is being resolved. Consider expanding the comment to clarify which functions were conflicting and why the generic:: qualification is necessary.

Suggested change
// Avoid ambiguous call due to ADL
// Avoid ambiguous call due to ADL: There are multiple 'store' functions in different namespaces,
// including a global 'store' and 'generic::store'. Without the 'generic::' qualification,
// the compiler may be unable to resolve which 'store' to use due to argument-dependent lookup (ADL),
// leading to ambiguity. Explicitly qualifying with 'generic::' ensures the correct function is called.

Copilot uses AI. Check for mistakes.
generic::store<T>(dst, splat<T>(value));
} else if constexpr (is_array_v<T>) {
using value_type = typename T::value_type;
const auto Splat = splat<value_type>(value);
Expand Down
2 changes: 1 addition & 1 deletion libc/test/UnitTest/FEnvSafeTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ void FEnvSafeTest::set_fenv(const fenv_t &fenv) {

void FEnvSafeTest::expect_fenv_eq(const fenv_t &before_fenv,
const fenv_t &after_fenv) {
#if defined(LIBC_TARGET_ARCH_IS_AARCH64)
#if defined(LIBC_TARGET_ARCH_IS_AARCH64) && !defined(LIBC_COMPILER_IS_MSVC)
using FPState = LIBC_NAMESPACE::fputil::FEnv::FPState;
const FPState &before_state = reinterpret_cast<const FPState &>(before_fenv);
const FPState &after_state = reinterpret_cast<const FPState &>(after_fenv);
Expand Down
Loading