Fix instruction overflow/underflow problem
There are undefined behaviors of integer type overflow/underflow in
arithmetic/logic operations.
Cast the inputs to the proper data type before the operation to avoid
undefined behaviors.
Modify the test input vectors to ensure the min/max of the input type is covered.
PiperOrigin-RevId: 551429757
diff --git a/sim/kelvin_vector_instructions.cc b/sim/kelvin_vector_instructions.cc
index 5470b24..a780422 100644
--- a/sim/kelvin_vector_instructions.cc
+++ b/sim/kelvin_vector_instructions.cc
@@ -150,9 +150,15 @@
template <typename T>
void KelvinVAdd(bool scalar, bool strip_mine, Instruction *inst) {
// Return vs1 + vs2.
- KelvinBinaryVectorOp(
- inst, scalar, strip_mine,
- std::function<T(T, T)>([](T vs1, T vs2) -> T { return vs1 + vs2; }));
+ KelvinBinaryVectorOp(inst, scalar, strip_mine,
+ std::function<T(T, T)>([](T vs1, T vs2) -> T {
+ using UT = typename std::make_unsigned<T>::type;
+ // Cast to unsigned type before the operation to avoid
+ // undefined overflow behavior in intx_t.
+ UT uvs1 = static_cast<UT>(vs1);
+ UT uvs2 = static_cast<UT>(vs2);
+ return static_cast<T>(uvs1 + uvs2);
+ }));
}
template void KelvinVAdd<int8_t>(bool, bool, Instruction *);
template void KelvinVAdd<int16_t>(bool, bool, Instruction *);
@@ -161,9 +167,15 @@
template <typename T>
void KelvinVSub(bool scalar, bool strip_mine, Instruction *inst) {
// Return vs1 - vs2.
- KelvinBinaryVectorOp(
- inst, scalar, strip_mine,
- std::function<T(T, T)>([](T vs1, T vs2) -> T { return vs1 - vs2; }));
+ KelvinBinaryVectorOp(inst, scalar, strip_mine,
+ std::function<T(T, T)>([](T vs1, T vs2) -> T {
+ using UT = typename std::make_unsigned<T>::type;
+ // Cast to unsigned type before the operation to avoid
+ // undefined overflow behavior in intx_t.
+ UT uvs1 = static_cast<UT>(vs1);
+ UT uvs2 = static_cast<UT>(vs2);
+ return static_cast<T>(uvs1 - uvs2);
+ }));
}
template void KelvinVSub<int8_t>(bool, bool, Instruction *);
template void KelvinVSub<int16_t>(bool, bool, Instruction *);
@@ -172,9 +184,15 @@
template <typename T>
void KelvinVRSub(bool scalar, bool strip_mine, Instruction *inst) {
// Return vs2 - vs1.
- KelvinBinaryVectorOp(
- inst, scalar, strip_mine,
- std::function<T(T, T)>([](T vs1, T vs2) -> T { return vs2 - vs1; }));
+ KelvinBinaryVectorOp(inst, scalar, strip_mine,
+ std::function<T(T, T)>([](T vs1, T vs2) -> T {
+ using UT = typename std::make_unsigned<T>::type;
+ // Cast to unsigned type before the operation to avoid
+ // undefined overflow behavior in intx_t.
+ UT uvs1 = static_cast<UT>(vs1);
+ UT uvs2 = static_cast<UT>(vs2);
+ return static_cast<T>(uvs2 - uvs1);
+ }));
}
template void KelvinVRSub<int8_t>(bool, bool, Instruction *);
template void KelvinVRSub<int16_t>(bool, bool, Instruction *);
@@ -317,8 +335,13 @@
// Return the summation of vd, vs1, and vs2.
KelvinBinaryVectorOp<false /* halftype */, false /* widen_dst */, T, T, T, T>(
inst, scalar, strip_mine,
- std::function<T(T, T, T)>(
- [](T vd, T vs1, T vs2) -> T { return vd + vs1 + vs2; }));
+ std::function<T(T, T, T)>([](T vd, T vs1, T vs2) -> T {
+ using UT = typename std::make_unsigned<T>::type;
+ UT uvs1 = static_cast<UT>(vs1);
+ UT uvs2 = static_cast<UT>(vs2);
+ UT uvd = static_cast<UT>(vd);
+ return static_cast<T>(uvd + uvs1 + uvs2);
+ }));
}
template void KelvinVAdd3<int8_t>(bool, bool, Instruction *);
template void KelvinVAdd3<int16_t>(bool, bool, Instruction *);
@@ -437,10 +460,12 @@
template <typename Td, typename Ts2>
void KelvinVAcc(bool scalar, bool strip_mine, Instruction *inst) {
// Accumulates operands with widening.
- KelvinBinaryVectorOp(inst, scalar, strip_mine,
- std::function<Td(Td, Ts2)>([](Td vs1, Ts2 vs2) -> Td {
- return vs1 + static_cast<Td>(vs2);
- }));
+ KelvinBinaryVectorOp(
+ inst, scalar, strip_mine,
+ std::function<Td(Td, Ts2)>([](Td vs1, Ts2 vs2) -> Td {
+ using UTd = typename std::make_unsigned<Td>::type;
+ return static_cast<Td>(static_cast<UTd>(vs1) + static_cast<UTd>(vs2));
+ }));
}
template void KelvinVAcc<int16_t, int8_t>(bool, bool, Instruction *);
template void KelvinVAcc<int32_t, int16_t>(bool, bool, Instruction *);
@@ -712,8 +737,11 @@
} else if (shamt > 0) {
s = (static_cast<int64_t>(vs1) + (round ? (1ll << (shamt - 1)) : 0)) >>
shamt;
- } else {
- s = static_cast<int64_t>(vs1) << (-shamt);
+ } else { // shamt < 0
+ using UT = typename std::make_unsigned<T>::type;
+ UT ushamt = static_cast<UT>(-shamt <= n ? -shamt : n);
+ CHECK(ushamt >= 0 && ushamt <= n);
+ s = static_cast<int64_t>(static_cast<uint64_t>(vs1) << ushamt);
}
T neg_max = std::numeric_limits<T>::min();
T pos_max = std::numeric_limits<T>::max();
@@ -724,6 +752,7 @@
return s;
} else {
constexpr int n = sizeof(T) * 8;
+ // Shift can be positive/negative.
int shamt = static_cast<typename std::make_signed<T>::type>(vs2);
uint64_t s = vs1;
if (!vs1) {
@@ -731,10 +760,12 @@
} else if (shamt > n) {
s = 0;
} else if (shamt > 0) {
- s = (static_cast<int64_t>(vs1) + (round ? (1ull << (shamt - 1)) : 0)) >>
+ s = (static_cast<uint64_t>(vs1) + (round ? (1ull << (shamt - 1)) : 0)) >>
shamt;
} else {
- s = static_cast<int64_t>(vs1) << (-shamt);
+ using UT = typename std::make_unsigned<T>::type;
+ UT ushamt = static_cast<UT>(-shamt <= n ? -shamt : n);
+ s = static_cast<uint64_t>(vs1) << (ushamt);
}
T pos_max = std::numeric_limits<T>::max();
bool pos_sat = vs1 && (shamt < -n || s > pos_max);
@@ -869,9 +900,16 @@
// Multiplication of vector elements.
template <typename T>
void KelvinVMul(bool scalar, bool strip_mine, Instruction *inst) {
- KelvinBinaryVectorOp(
- inst, scalar, strip_mine,
- std::function<T(T, T)>([](T vs1, T vs2) -> T { return vs1 * vs2; }));
+ KelvinBinaryVectorOp(inst, scalar, strip_mine,
+ std::function<T(T, T)>([](T vs1, T vs2) -> T {
+ if (std::is_signed<T>::value) {
+ return static_cast<T>(static_cast<int64_t>(vs1) *
+ static_cast<int64_t>(vs2));
+ } else {
+ return static_cast<T>(static_cast<uint64_t>(vs1) *
+ static_cast<uint64_t>(vs2));
+ }
+ }));
}
template void KelvinVMul<int8_t>(bool, bool, Instruction *);
template void KelvinVMul<int16_t>(bool, bool, Instruction *);
diff --git a/sim/test/kelvin_vector_instructions_test.cc b/sim/test/kelvin_vector_instructions_test.cc
index 6aede99..8b5ed9c 100644
--- a/sim/test/kelvin_vector_instructions_test.cc
+++ b/sim/test/kelvin_vector_instructions_test.cc
@@ -321,7 +321,11 @@
// Vector add.
template <typename Vd, typename Vs1, typename Vs2>
struct VAddOp {
- static Vd Op(Vs1 vs1, Vs2 vs2) { return vs1 + vs2; }
+ static Vd Op(Vs1 vs1, Vs2 vs2) {
+ int64_t vs1_ext = static_cast<int64_t>(vs1);
+ int64_t vs2_ext = static_cast<int64_t>(vs2);
+ return static_cast<Vd>(vs1_ext + vs2_ext);
+ }
static void KelvinOp(bool scalar, bool strip_mine, Instruction *inst) {
KelvinVAdd<Vd>(scalar, strip_mine, inst);
}
@@ -333,7 +337,11 @@
// Vector subtract.
template <typename Vd, typename Vs1, typename Vs2>
struct VSubOp {
- static Vd Op(Vs1 vs1, Vs2 vs2) { return vs1 - vs2; }
+ static Vd Op(Vs1 vs1, Vs2 vs2) {
+ int64_t vs1_ext = static_cast<int64_t>(vs1);
+ int64_t vs2_ext = static_cast<int64_t>(vs2);
+ return static_cast<Vd>(vs1_ext - vs2_ext);
+ }
static void KelvinOp(bool scalar, bool strip_mine, Instruction *inst) {
KelvinVSub<Vd>(scalar, strip_mine, inst);
}
@@ -345,7 +353,11 @@
// Vector reverse subtract.
template <typename Vd, typename Vs1, typename Vs2>
struct VRSubOp {
- static Vd Op(Vs1 vs1, Vs2 vs2) { return vs2 - vs1; }
+ static Vd Op(Vs1 vs1, Vs2 vs2) {
+ int64_t vs1_ext = static_cast<int64_t>(vs1);
+ int64_t vs2_ext = static_cast<int64_t>(vs2);
+ return static_cast<Vd>(vs2_ext - vs1_ext);
+ }
static void KelvinOp(bool scalar, bool strip_mine, Instruction *inst) {
KelvinVRSub<Vd>(scalar, strip_mine, inst);
}
@@ -507,7 +519,12 @@
// Vector add3.
template <typename Vd, typename Vs1, typename Vs2>
struct VAdd3Op {
- static Vd Op(Vd vd, Vs1 vs1, Vs2 vs2) { return vd + vs1 + vs2; }
+ static Vd Op(Vd vd, Vs1 vs1, Vs2 vs2) {
+ int64_t vs1_ext = static_cast<int64_t>(vs1);
+ int64_t vs2_ext = static_cast<int64_t>(vs2);
+ int64_t vd_ext = static_cast<int64_t>(vd);
+ return static_cast<Vd>(vd_ext + vs1_ext + vs2_ext);
+ }
static void KelvinOp(bool scalar, bool strip_mine, Instruction *inst) {
KelvinVAdd3<Vd>(scalar, strip_mine, inst);
}
@@ -647,7 +664,11 @@
// Vector accumulate with widening.
template <typename Vd, typename Vs1, typename Vs2>
struct VAccOp {
- static Vd Op(Vd vs1, Vs2 vs2) { return vs1 + static_cast<Vd>(vs2); }
+ static Vd Op(Vd vs1, Vs2 vs2) {
+ int64_t vs1_ext = static_cast<int64_t>(vs1);
+ int64_t vs2_ext = static_cast<int64_t>(vs2);
+ return static_cast<Vd>(vs1_ext + vs2_ext);
+ }
static void KelvinOp(bool scalar, bool strip_mine, Instruction *inst) {
KelvinVAcc<Vd, Vs2>(scalar, strip_mine, inst);
}
@@ -999,8 +1020,9 @@
} else if (shamt > 0) {
s = (static_cast<int64_t>(vs1) + (round ? (1ll << (shamt - 1)) : 0)) >>
shamt;
- } else {
- s = static_cast<int64_t>(vs1) << (-shamt);
+ } else { // shamt < 0
+ uint32_t ushamt = static_cast<uint32_t>(-shamt <= n ? -shamt : n);
+ s = static_cast<int64_t>(static_cast<uint64_t>(vs1) << ushamt);
}
int64_t neg_max = (-1ull) << (n - 1);
int64_t pos_max = (1ll << (n - 1)) - 1;
@@ -1025,7 +1047,9 @@
(round ? (1ull << (shamt - 1)) : 0)) >>
shamt;
} else {
- s = static_cast<uint64_t>(vs1) << (-shamt);
+ using UT = typename std::make_unsigned<Vd>::type;
+ UT ushamt = static_cast<UT>(-shamt <= n ? -shamt : n);
+ s = static_cast<uint64_t>(vs1) << (ushamt);
}
uint64_t pos_max = (1ull << n) - 1;
bool pos_sat = vs1 && (shamt < -n || s >= (1ull << n));
@@ -1207,7 +1231,15 @@
// Vector elements multiplication.
template <typename Vd, typename Vs1, typename Vs2>
struct VMulOp {
- static Vd Op(Vs1 vs1, Vs2 vs2) { return vs1 * vs2; }
+ static Vd Op(Vs1 vs1, Vs2 vs2) {
+ if (std::is_signed<Vd>::value) {
+ return static_cast<Vd>(static_cast<int64_t>(vs1) *
+ static_cast<int64_t>(vs2));
+ } else {
+ return static_cast<Vd>(static_cast<uint64_t>(vs1) *
+ static_cast<uint64_t>(vs2));
+ }
+ }
static void KelvinOp(bool scalar, bool strip_mine, Instruction *inst) {
KelvinVMul<Vd>(scalar, strip_mine, inst);
}
diff --git a/sim/test/kelvin_vector_instructions_test_base.h b/sim/test/kelvin_vector_instructions_test_base.h
index 7335a28..6f3170e 100644
--- a/sim/test/kelvin_vector_instructions_test_base.h
+++ b/sim/test/kelvin_vector_instructions_test_base.h
@@ -174,8 +174,13 @@
int vs1_size = vector_length_in_bytes / sizeof(Vs1);
const size_t vs1_regs_count = num_ops * src1_widen_factor;
std::vector<Vs1> vs1_value(vs1_size * vs1_regs_count);
+ // Use the first 4 elements to check the min/max boundary behavior
+ vs1_value[0] = std::numeric_limits<Vs1>::lowest();
+ vs1_value[1] = std::numeric_limits<Vs1>::lowest();
+ vs1_value[2] = std::numeric_limits<Vs1>::max();
+ vs1_value[3] = std::numeric_limits<Vs1>::max();
auto vs1_span = absl::Span<Vs1>(vs1_value);
- FillArrayWithRandomValues<Vs1>(vs1_span);
+ FillArrayWithRandomValues<Vs1>(vs1_span.subspan(4, vs1_span.size() - 4));
for (int i = 0; i < vs1_regs_count; i++) {
auto vs1_name = absl::StrCat("v", kVs1 + i);
SetVectorRegisterValues<Vs1>(
@@ -196,8 +201,14 @@
static_cast<typename mpact::sim::riscv::SameSignedType<
RV32Register::ValueType, Ts2>::type>(rs2_reg_value));
} else if (!halftype_op) {
+ // Use the value slightly greater than min so VShift won't complain
+ // -shamt.
+ vs2_value[0] = std::numeric_limits<Ts2>::lowest() + 1;
+ vs2_value[1] = std::numeric_limits<Ts2>::max();
+ vs2_value[2] = std::numeric_limits<Ts2>::lowest() + 1;
+ vs2_value[3] = std::numeric_limits<Ts2>::max();
auto vs2_span = absl::Span<Ts2>(vs2_value);
- FillArrayWithRandomValues<Ts2>(vs2_span);
+ FillArrayWithRandomValues<Ts2>(vs2_span.subspan(4, vs2_span.size() - 4));
for (int i = 0; i < num_ops; i++) {
auto vs2_name = absl::StrCat("v", kVs2 + i);
SetVectorRegisterValues<Ts2>(
@@ -209,8 +220,12 @@
const size_t vd_size = vector_length_in_bytes / sizeof(Vd);
const size_t dest_regs_count = num_ops * dest_regs_per_op;
std::vector<Vd> vd_value(vd_size * dest_regs_count);
+ vd_value[0] = std::numeric_limits<Vd>::lowest();
+ vd_value[1] = std::numeric_limits<Vd>::max();
+ vd_value[2] = std::numeric_limits<Vd>::lowest();
+ vd_value[3] = std::numeric_limits<Vd>::max();
auto vd_span = absl::Span<Vd>(vd_value);
- FillArrayWithRandomValues<Vd>(vd_span);
+ FillArrayWithRandomValues<Vd>(vd_span.subspan(4, vd_span.size() - 4));
for (int i = 0; i < dest_regs_count; i++) {
auto vd_name = absl::StrCat("v", kVd + i);
SetVectorRegisterValues<Vd>(
