Skip to content

Commit fdf2180

Browse files
danakjdanakj
committed
Fix 64bit multiplication in non-constexpr context on MSVC
We were using _mul128 but even though it works on signed integers it does not properly multiply signed integers, the sign bit is not used correctly. So instead use the constexpr version of the function which was manually checking for overflow and then multiplying on MSVC.
1 parent eea4ccd commit fdf2180

File tree

5 files changed

+76
-31
lines changed

5 files changed

+76
-31
lines changed

‎subspace/num/__private/intrinsics.h

Lines changed: 22 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -732,37 +732,28 @@ template <class T>
732732
sus_always_inline constexpr OverflowOut<T> mul_with_overflow(T x,
733733
T y) noexcept {
734734
#if _MSC_VER
735-
if (std::is_constant_evaluated()) {
736-
if (x == T{0} || y == T{0})
737-
return OverflowOut sus_clang_bug_56394(<T>){.overflow = false,
738-
.value = T{0}};
739-
740-
using U = decltype(into_unsigned(x));
741-
const auto absx =
742-
x >= T{0} ? into_unsigned(x)
743-
: unchecked_add(into_unsigned(unchecked_add(x, T{1})), U{1});
744-
const auto absy =
745-
y >= T{0} ? into_unsigned(y)
746-
: unchecked_add(into_unsigned(unchecked_add(y, T{1})), U{1});
747-
const bool mul_negative = (x ^ y) < 0;
748-
const auto mul_max =
749-
unchecked_add(into_unsigned(max_value<T>()), U{mul_negative});
750-
const bool overflow = absx > unchecked_div(mul_max, absy);
751-
const auto mul_val = unchecked_mul(absx, absy);
752-
return OverflowOut sus_clang_bug_56394(<T>){
753-
.overflow = overflow,
754-
.value = mul_negative
755-
? unchecked_sub(unchecked_neg(static_cast<T>(mul_val - 1)),
756-
T{1})
757-
: static_cast<T>(mul_val)};
758-
} else {
759-
// For MSVC, use _mul128, but what about constexpr?? If we can't do
760-
// it then make the whole function non-constexpr?
761-
int64_t highbits;
762-
auto out = static_cast<T>(_mul128(x, y, &highbits));
763-
return OverflowOut sus_clang_bug_56394(<T>){.overflow = highbits != 0,
764-
.value = out};
765-
}
735+
if (x == T{0} || y == T{0})
736+
return OverflowOut sus_clang_bug_56394(<T>){.overflow = false,
737+
.value = T{0}};
738+
739+
using U = decltype(into_unsigned(x));
740+
const auto absx =
741+
x >= T{0}
742+
? into_unsigned(x)
743+
: unchecked_add(into_unsigned(unchecked_neg(unchecked_add(x, T{1}))),
744+
U{1});
745+
const auto absy =
746+
y >= T{0}
747+
? into_unsigned(y)
748+
: unchecked_add(into_unsigned(unchecked_neg(unchecked_add(y, T{1}))),
749+
U{1});
750+
const bool mul_negative = (x ^ y) < 0;
751+
const auto mul_max =
752+
unchecked_add(into_unsigned(max_value<T>()), U{mul_negative});
753+
const bool overflow = absx > unchecked_div(mul_max, absy);
754+
const auto mul_val = unchecked_mul(into_unsigned(x), into_unsigned(y));
755+
return OverflowOut sus_clang_bug_56394(<T>){.overflow = overflow,
756+
.value = into_signed(mul_val)};
766757
#else
767758
auto out = __int128_t{x} * __int128_t{y};
768759
return OverflowOut sus_clang_bug_56394(<T>){

‎subspace/num/i32_unittest.cc

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -885,6 +885,22 @@ TEST(i32DeathTest, MulOverflow) {
885885
#endif
886886
}
887887

888+
TEST(i32, CheckedMul) {
889+
constexpr auto a = (1_i32).checked_mul(3_i32).unwrap();
890+
EXPECT_EQ(a, 3_i32);
891+
892+
EXPECT_EQ((100_i32).checked_mul(21_i32), sus::some(2100_i32).construct());
893+
EXPECT_EQ((21_i32).checked_mul(100_i32), sus::some(2100_i32).construct());
894+
EXPECT_EQ((123456_i32).checked_mul(234567_i32), sus::None);
895+
896+
// ** Signed only.
897+
EXPECT_EQ((-3_i32).checked_mul(10_i32), sus::some(-30_i32).construct());
898+
EXPECT_EQ((-100_i32).checked_mul(21_i32), sus::some(-2100_i32).construct());
899+
EXPECT_EQ((-21_i32).checked_mul(100_i32), sus::some(-2100_i32).construct());
900+
EXPECT_EQ((-123456_i32).checked_mul(-23456_i32), sus::None);
901+
EXPECT_EQ((123456_i32).checked_mul(-23456_i32), sus::None);
902+
}
903+
888904
TEST(i32, OverflowingMul) {
889905
constexpr auto a = (123456_i32).overflowing_mul(234567_i32);
890906
EXPECT_EQ(
@@ -1392,6 +1408,8 @@ TEST(i32, CheckedSub) {
13921408
EXPECT_EQ(i32::MIN.checked_sub(i32::MAX), None);
13931409

13941410
// ** Signed only.
1411+
EXPECT_EQ((-12345_i32).checked_sub(3_i32).unwrap(), -12348_i32);
1412+
EXPECT_EQ((12345_i32).checked_sub(-3_i32).unwrap(), 12348_i32);
13951413
EXPECT_EQ((-12345_i32).checked_sub(-12345_i32).unwrap(), 0_i32);
13961414
EXPECT_EQ(i32::MAX.checked_sub(-1_i32), None);
13971415
EXPECT_EQ((-2_i32).checked_sub(i32::MAX), None);

‎subspace/num/i64_unittest.cc

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
#include "subspace/num/__private/intrinsics.h"
12
// Copyright 2022 Google LLC
23
//
34
// Licensed under the Apache License, Version 2.0 (the "License");
@@ -341,6 +342,22 @@ TEST(i64DeathTest, FromOutOfRange) {
341342
#endif
342343
}
343344

345+
TEST(i64, CheckedMul) {
346+
constexpr auto a = (1_i64).checked_mul(3_i64).unwrap();
347+
EXPECT_EQ(a, 3_i64);
348+
349+
EXPECT_EQ((100_i64).checked_mul(21_i64), sus::some(2100_i64).construct());
350+
EXPECT_EQ((21_i64).checked_mul(100_i64), sus::some(2100_i64).construct());
351+
EXPECT_EQ((i64::MAX).checked_mul(2_i64), sus::None);
352+
353+
// ** Signed only.
354+
EXPECT_EQ((-3_i64).checked_mul(10_i64), sus::some(-30_i64).construct());
355+
EXPECT_EQ((-100_i64).checked_mul(21_i64), sus::some(-2100_i64).construct());
356+
EXPECT_EQ((-21_i64).checked_mul(100_i64), sus::some(-2100_i64).construct());
357+
EXPECT_EQ((i64::MIN).checked_mul(-2_i64), sus::None);
358+
EXPECT_EQ((i64::MAX).checked_mul(-2_i64), sus::None);
359+
}
360+
344361
TEST(i64, InvokeEverything) {
345362
auto i = 10_i64, j = 11_i64;
346363
auto s = 3_u64;

‎subspace/num/u32_unittest.cc

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -631,6 +631,15 @@ TEST(u32DeathTest, MulOverflow) {
631631
#endif
632632
}
633633

634+
TEST(u32, CheckedMul) {
635+
constexpr auto a = (1_u32).checked_mul(3_u32).unwrap();
636+
EXPECT_EQ(a, 3_u32);
637+
638+
EXPECT_EQ((100_u32).checked_mul(21_u32), sus::some(2100_u32).construct());
639+
EXPECT_EQ((21_u32).checked_mul(100_u32), sus::some(2100_u32).construct());
640+
EXPECT_EQ((123456_u32).checked_mul(234567_u32), sus::None);
641+
}
642+
634643
TEST(u32, OverflowingMul) {
635644
constexpr auto a = (123456_u32).overflowing_mul(234567_u32);
636645
EXPECT_EQ(

‎subspace/num/u64_unittest.cc

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -355,6 +355,16 @@ TEST(u64DeathTest, FromOutOfRange) {
355355
#endif
356356
}
357357

358+
TEST(u64, CheckedMul) {
359+
constexpr auto a = (1_u64).checked_mul(3_u64).unwrap();
360+
EXPECT_EQ(a, 3_u64);
361+
362+
EXPECT_EQ((100_u64).checked_mul(21_u64), sus::some(2100_u64).construct());
363+
EXPECT_EQ((21_u64).checked_mul(100_u64), sus::some(2100_u64).construct());
364+
EXPECT_EQ((u64::MAX).checked_mul(2_u64), sus::None);
365+
}
366+
367+
358368
TEST(u64, InvokeEverything) {
359369
auto i = 10_u64, j = 11_u64;
360370
auto s = 3_i64;

0 commit comments

Comments
 (0)