From 7cbe6ed048ab647f2be7ba11173bda3251d68a41 Mon Sep 17 00:00:00 2001 From: George Rennie Date: Wed, 7 May 2025 14:36:48 +0200 Subject: [PATCH 1/6] kernel: add safer variants of as_int --- kernel/rtlil.cc | 56 +++++++++++++++++++++++++++++++++++++++++++++++++ kernel/rtlil.h | 6 ++++++ 2 files changed, 62 insertions(+) diff --git a/kernel/rtlil.cc b/kernel/rtlil.cc index dd78b202d..9f5972c32 100644 --- a/kernel/rtlil.cc +++ b/kernel/rtlil.cc @@ -380,6 +380,30 @@ int RTLIL::Const::as_int(bool is_signed) const return ret; } +bool RTLIL::Const::convertible_to_int(bool is_signed) const +{ + auto size = get_min_size(is_signed); + return (size > 0 && size <= 32); +} + +std::optional RTLIL::Const::try_as_int(bool is_signed) const +{ + if (!convertible_to_int(is_signed)) + return std::nullopt; + return as_int(is_signed); +} + +int RTLIL::Const::as_int_saturating(bool is_signed) const +{ + if (!convertible_to_int(is_signed)) { + const auto min_size = get_min_size(is_signed); + log_assert(min_size > 0); + const auto neg = get_bits().at(min_size - 1); + return neg ? std::numeric_limits::min() : std::numeric_limits::max(); + } + return as_int(is_signed); +} + int RTLIL::Const::get_min_size(bool is_signed) const { if (empty()) return 0; @@ -5462,6 +5486,38 @@ int RTLIL::SigSpec::as_int(bool is_signed) const return 0; } +bool RTLIL::SigSpec::convertible_to_int(bool is_signed) const +{ + cover("kernel.rtlil.sigspec.convertible_to_int"); + + pack(); + if (!is_fully_const()) + return false; + + return RTLIL::Const(chunks_[0].data).convertible_to_int(is_signed); +} + +std::optional RTLIL::SigSpec::try_as_int(bool is_signed) const +{ + cover("kernel.rtlil.sigspec.try_as_int"); + + pack(); + if (!is_fully_const()) + return std::nullopt; + + return RTLIL::Const(chunks_[0].data).try_as_int(is_signed); +} + +int RTLIL::SigSpec::as_int_saturating(bool is_signed) const +{ + cover("kernel.rtlil.sigspec.try_as_int"); + + pack(); + log_assert(is_fully_const() && GetSize(chunks_) <= 1); + log_assert(!empty()); + return RTLIL::Const(chunks_[0].data).as_int_saturating(is_signed); +} + std::string RTLIL::SigSpec::as_string() const { cover("kernel.rtlil.sigspec.as_string"); diff --git a/kernel/rtlil.h b/kernel/rtlil.h index 96c8c523b..50c96c71b 100644 --- a/kernel/rtlil.h +++ b/kernel/rtlil.h @@ -754,6 +754,9 @@ public: std::vector& bits(); bool as_bool() const; int as_int(bool is_signed = false) const; + bool convertible_to_int(bool is_signed = false) const; + std::optional try_as_int(bool is_signed = false) const; + int as_int_saturating(bool is_signed = false) const; std::string as_string(const char* any = "-") const; static Const from_string(const std::string &str); std::vector to_bits() const; @@ -1131,6 +1134,9 @@ public: bool as_bool() const; int as_int(bool is_signed = false) const; + bool convertible_to_int(bool is_signed = false) const; + std::optional try_as_int(bool is_signed = false) const; + int as_int_saturating(bool is_signed = false) const; std::string as_string() const; RTLIL::Const as_const() const; RTLIL::Wire *as_wire() const; From 0dcd94b6ad22612e884cb55337c1ec0441715805 Mon Sep 17 00:00:00 2001 From: George Rennie Date: Wed, 7 May 2025 14:41:13 +0200 Subject: [PATCH 2/6] opt_expr: saturate shift amount instead of overflowing for large shifts --- passes/opt/opt_expr.cc | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/passes/opt/opt_expr.cc b/passes/opt/opt_expr.cc index 74f5b386a..1c8bb4feb 100644 --- a/passes/opt/opt_expr.cc +++ b/passes/opt/opt_expr.cc @@ -1307,7 +1307,12 @@ skip_fine_alu: if (cell->type.in(ID($shl), ID($shr), ID($sshl), ID($sshr), ID($shift), ID($shiftx)) && (keepdc ? assign_map(cell->getPort(ID::B)).is_fully_def() : assign_map(cell->getPort(ID::B)).is_fully_const())) { bool sign_ext = cell->type == ID($sshr) && cell->getParam(ID::A_SIGNED).as_bool(); - int shift_bits = assign_map(cell->getPort(ID::B)).as_int(cell->type.in(ID($shift), ID($shiftx)) && cell->getParam(ID::B_SIGNED).as_bool()); + RTLIL::SigSpec sig_b = assign_map(cell->getPort(ID::B)); + const bool b_sign_ext = cell->type.in(ID($shift), ID($shiftx)) && cell->getParam(ID::B_SIGNED).as_bool(); + // We saturate the value to prevent overflow, but note that this could + // cause incorrect opimization in the impractical case that A is 2^32 bits + // wide + int shift_bits = sig_b.as_int_saturating(b_sign_ext); if (cell->type.in(ID($shl), ID($sshl))) shift_bits *= -1; From af933b4f3801fa560e0ec4a4f33f99ff41265920 Mon Sep 17 00:00:00 2001 From: George Rennie Date: Wed, 7 May 2025 15:12:33 +0200 Subject: [PATCH 3/6] tests: check shifts by amounts that overflow int --- tests/opt/opt_expr_shift.ys | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/tests/opt/opt_expr_shift.ys b/tests/opt/opt_expr_shift.ys index aac2e6f62..5944bfa33 100644 --- a/tests/opt/opt_expr_shift.ys +++ b/tests/opt/opt_expr_shift.ys @@ -48,3 +48,25 @@ select -assert-none t:$shl select -assert-none t:$shr select -assert-none t:$sshl select -assert-none t:$sshr + +design -reset + +read_verilog <> 36'hfffffffff); + wire signed [35:0] shamt = 36'hfffffffff; + assign out2 = (in >> shamt); +endmodule +EOT + +equiv_opt opt_expr + +design -load postopt +select -assert-none t:$shl +select -assert-none t:$shr +select -assert-none t:$sshl +select -assert-none t:$sshr From e2485000c7d7158097be01e29a6e18d7fe899d00 Mon Sep 17 00:00:00 2001 From: George Rennie Date: Thu, 8 May 2025 11:08:20 +0200 Subject: [PATCH 4/6] kernel: handle unsigned case for as_int_saturating correctly * This fixes #5105 --- kernel/rtlil.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/kernel/rtlil.cc b/kernel/rtlil.cc index 9f5972c32..c08e78dce 100644 --- a/kernel/rtlil.cc +++ b/kernel/rtlil.cc @@ -396,6 +396,9 @@ std::optional RTLIL::Const::try_as_int(bool is_signed) const int RTLIL::Const::as_int_saturating(bool is_signed) const { if (!convertible_to_int(is_signed)) { + if (!is_signed) + return std::numeric_limits::max(); + const auto min_size = get_min_size(is_signed); log_assert(min_size > 0); const auto neg = get_bits().at(min_size - 1); From d59380b3a0e6596f141e8f96de1b58b7540385f2 Mon Sep 17 00:00:00 2001 From: George Rennie Date: Thu, 8 May 2025 11:09:01 +0200 Subject: [PATCH 5/6] tests: more complete testing of shift edgecases --- tests/opt/opt_expr_shift.ys | 55 ++++++++++++++++++++++++++++++++----- 1 file changed, 48 insertions(+), 7 deletions(-) diff --git a/tests/opt/opt_expr_shift.ys b/tests/opt/opt_expr_shift.ys index 5944bfa33..943d370dc 100644 --- a/tests/opt/opt_expr_shift.ys +++ b/tests/opt/opt_expr_shift.ys @@ -20,7 +20,11 @@ module top ( output wire [7:0] sshr_uu, output wire signed [7:0] sshr_us, output wire [7:0] sshr_su, - output wire signed [7:0] sshr_ss + output wire signed [7:0] sshr_ss, + output wire [7:0] shiftx_uu, + output wire signed [7:0] shiftx_us, + output wire [7:0] shiftx_su, + output wire signed [7:0] shiftx_ss ); assign shl_uu = in_u << 20; assign shl_us = in_u << 20; @@ -38,9 +42,20 @@ module top ( assign sshr_us = in_u >>> 20; assign sshr_su = in_s >>> 20; assign sshr_ss = in_s >>> 20; + wire [7:0] shamt = 20; + assign shiftx_uu = in_u[shamt +: 8]; + assign shiftx_us = in_u[shamt +: 8]; + assign shiftx_su = in_s[shamt +: 8]; + assign shiftx_ss = in_s[shamt +: 8]; endmodule EOT +select -assert-count 4 t:$shl +select -assert-count 4 t:$shr +select -assert-count 4 t:$sshl +select -assert-count 4 t:$sshr +select -assert-count 4 t:$shiftx + equiv_opt opt_expr design -load postopt @@ -48,21 +63,46 @@ select -assert-none t:$shl select -assert-none t:$shr select -assert-none t:$sshl select -assert-none t:$sshr +select -assert-none t:$shiftx design -reset read_verilog <> 36'hfffffffff; + assign sshl = in <<< 36'hfffffffff; + assign sshr = in >>> 36'hfffffffff; + assign shiftx = in[36'hfffffffff +: 8]; - assign out1 = (in >> 36'hfffffffff); wire signed [35:0] shamt = 36'hfffffffff; - assign out2 = (in >> shamt); + assign shl_s = in << shamt; + assign shr_s = in >> shamt; + assign sshl_s = in <<< shamt; + assign sshr_s = in >>> shamt; + assign shiftx_s = in[shamt +: 8]; endmodule EOT +select -assert-count 2 t:$shl +select -assert-count 2 t:$shr +select -assert-count 2 t:$sshl +select -assert-count 2 t:$sshr +select -assert-count 1 t:$shiftx + equiv_opt opt_expr design -load postopt @@ -70,3 +110,4 @@ select -assert-none t:$shl select -assert-none t:$shr select -assert-none t:$sshl select -assert-none t:$sshr +select -assert-none t:$shiftx From 98eec369210302a6d2aee856a00a571d019a1b00 Mon Sep 17 00:00:00 2001 From: George Rennie Date: Wed, 21 May 2025 12:20:08 +0100 Subject: [PATCH 6/6] kernel: add comments to as_int family of methods --- kernel/rtlil.h | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/kernel/rtlil.h b/kernel/rtlil.h index 50c96c71b..fb9efca51 100644 --- a/kernel/rtlil.h +++ b/kernel/rtlil.h @@ -753,10 +753,26 @@ public: std::vector& bits(); bool as_bool() const; + + // Convert the constant value to a C++ int. + // NOTE: If the constant is too wide to fit in int (32 bits) this will + // truncate any higher bits, potentially over/underflowing. Consider using + // try_as_int, as_int_saturating, or guarding behind convertible_to_int + // instead. int as_int(bool is_signed = false) const; + + // Returns true iff the constant can be converted to an int without + // over/underflow. bool convertible_to_int(bool is_signed = false) const; + + // Returns the constant's value as an int if it can be represented without + // over/underflow, or std::nullopt otherwise. std::optional try_as_int(bool is_signed = false) const; + + // Returns the constant's value as an int if it can be represented without + // over/underflow, otherwise the max/min value for int depending on the sign. int as_int_saturating(bool is_signed = false) const; + std::string as_string(const char* any = "-") const; static Const from_string(const std::string &str); std::vector to_bits() const; @@ -1133,10 +1149,27 @@ public: bool is_onehot(int *pos = nullptr) const; bool as_bool() const; + + // Convert the SigSpec to a C++ int, assuming all bits are constant. + // NOTE: If the value is too wide to fit in int (32 bits) this will + // truncate any higher bits, potentially over/underflowing. Consider using + // try_as_int, as_int_saturating, or guarding behind convertible_to_int + // instead. int as_int(bool is_signed = false) const; + + // Returns true iff the SigSpec is constant and can be converted to an int + // without over/underflow. bool convertible_to_int(bool is_signed = false) const; + + // Returns the SigSpec's value as an int if it is a constant and can be + // represented without over/underflow, or std::nullopt otherwise. std::optional try_as_int(bool is_signed = false) const; + + // Returns an all constant SigSpec's value as an int if it can be represented + // without over/underflow, otherwise the max/min value for int depending on + // the sign. int as_int_saturating(bool is_signed = false) const; + std::string as_string() const; RTLIL::Const as_const() const; RTLIL::Wire *as_wire() const;