From 121fc62affba775052c5208cd2736383918aa4f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Chmiel?= Date: Mon, 10 Aug 2026 13:52:18 +0200 Subject: [PATCH] Fix VPI cbValueChange for 1-bit select (#8063) --- include/verilated_vpi.cpp | 133 +++++++++++++++---- test_regress/t/t_vpi_var.cpp | 241 +++++++++++++++++++++++++++++++++++ test_regress/t/t_vpi_var.v | 6 + test_regress/t/t_vpi_var2.v | 6 + test_regress/t/t_vpi_var3.v | 6 + 5 files changed, 364 insertions(+), 28 deletions(-) diff --git a/include/verilated_vpi.cpp b/include/verilated_vpi.cpp index 65afe20f3..eab65d9c8 100644 --- a/include/verilated_vpi.cpp +++ b/include/verilated_vpi.cpp @@ -539,7 +539,7 @@ public: for (auto idx : index()) m_fullname += "[" + std::to_string(idx) + "]"; return m_fullname.c_str(); } - void* prevDatap() const { return m_prevDatap; } + uint8_t* prevDatap() const { return m_prevDatap; } void* varDatap() const override { return m_varDatap; } void createPrevDatap() { if (VL_UNLIKELY(!m_prevDatap)) { @@ -1027,6 +1027,26 @@ struct VerilatedVpiTimedCbsCmp final { class VerilatedVpiError; void vl_vpi_put_word(const VerilatedVpioVar* vop, QData word, size_t bitCount, size_t addOffset); +// Information about how to access packed array data. +// If underlying type is multi-word (VLVT_WDATA), the packed element might straddle word +// boundaries, in which case m_maskHi != 0. +template +struct VarAccessInfo final { + T* m_datap; // Typed pointer to packed array base address + size_t m_bitOffset; // Data start location (bit offset) + size_t m_wordOffset; // Data start location (word offset, VLVT_WDATA only) + T m_maskLo; // Access mask for m_datap[m_wordOffset] + T m_maskHi; // Access mask for m_datap[m_wordOffset + 1] (VLVT_WDATA only) +}; +template +VarAccessInfo vl_vpi_var_access_info(const VerilatedVpioVarBase* vop, size_t bitCount, + size_t addOffset); +template +T vl_vpi_get_word_gen(VarAccessInfo info); + +template +void vl_vpi_put_word_gen(VarAccessInfo info, T word); + class VerilatedVpiImp final { enum { CB_ENUM_MAX_VALUE = cbAtEndOfSimTime + 1 }; // Maximum callback reason using VpioCbList = std::list; @@ -1169,6 +1189,72 @@ public: s().m_cbCallList.clear(); return called; } + template + static bool valueDiffersFromPrev(VerilatedVpioVar* varop) { + VL_DEBUG_IF_PLI(VL_DBG_MSGF("- vpi: value_test %s v[0]=%d/%d %p %p size=%d\n", + varop->fullname(), *(static_cast(varop->varDatap())), + *(varop->prevDatap()), varop->varDatap(), varop->prevDatap(), + varop->entSize());); + if (varop->bitSize() == 1) { + T* const prevDatap = reinterpret_cast( + varop->prevDatap()); // Was malloced when we added the callback + const VarAccessInfo currInfo + = vl_vpi_var_access_info(varop, varop->bitSize(), 0); + VarAccessInfo prevInfo = currInfo; + prevInfo.m_datap = prevDatap; + return vl_vpi_get_word_gen(currInfo) != vl_vpi_get_word_gen(prevInfo); + } + return std::memcmp(varop->prevDatap(), varop->varDatap(), varop->entSize()) != 0; + } + static bool valueDiffersFromPrev(VerilatedVpioVar* varop) { + switch (varop->varp()->vltype()) { + case VLVT_UINT8: return valueDiffersFromPrev(varop); + case VLVT_UINT16: return valueDiffersFromPrev(varop); + case VLVT_UINT32: return valueDiffersFromPrev(varop); + case VLVT_UINT64: return valueDiffersFromPrev(varop); + case VLVT_WDATA: + return valueDiffersFromPrev(varop); + // LCOV_EXCL_START - Would require earlier type check to not catch that + default: + const std::string msg + = "Unsupported type (" + std::to_string(varop->varp()->vltype()) + ")"; + VL_FATAL_MT(__FILE__, __LINE__, "", msg.c_str()); + return true; + // LCOV_EXCL_STOP + } + } + template + static void updatePrev(const VerilatedVpioVar* const varop) { + if (varop->bitSize() == 1) { + const VarAccessInfo currInfo + = vl_vpi_var_access_info(varop, varop->bitSize(), 0); + VarAccessInfo prevInfo = currInfo; + T* const prevDatap = reinterpret_cast(varop->prevDatap()); + prevInfo.m_datap = prevDatap; + const T currWord = vl_vpi_get_word_gen(currInfo); + vl_vpi_put_word_gen(prevInfo, currWord); + assert(std::memcmp(varop->prevDatap(), varop->varDatap(), varop->entSize()) == 0); + } else { + std::memcpy(varop->prevDatap(), varop->varDatap(), varop->entSize()); + } + } + static void updatePrev(const VerilatedVpioVar* const varop) { + switch (varop->varp()->vltype()) { + case VLVT_UINT8: updatePrev(varop); break; + case VLVT_UINT16: updatePrev(varop); break; + case VLVT_UINT32: updatePrev(varop); break; + case VLVT_UINT64: updatePrev(varop); break; + case VLVT_WDATA: + updatePrev(varop); + break; + // LCOV_EXCL_START - Would require earlier type check to not catch that + default: + const std::string msg + = "Unsupported type (" + std::to_string(varop->varp()->vltype()) + ")"; + VL_FATAL_MT(__FILE__, __LINE__, "", msg.c_str()); + // LCOV_EXCL_STOP + } + } static bool callValueCbs() VL_MT_UNSAFE_ONE { assertOneCheck(); VpioCbList& cbObjList = s().m_cbCurrentLists[cbValueChange]; @@ -1187,15 +1273,10 @@ public: VerilatedVpiCbHolder& ho = *it++; VerilatedVpioVar* const varop = reinterpret_cast(ho.cb_datap()->obj); - void* const newDatap = varop->varDatap(); - void* const prevDatap = varop->prevDatap(); // Was malloced when we added the callback - VL_DEBUG_IF_PLI(VL_DBG_MSGF("- vpi: value_test %s v[0]=%d/%d %p %p\n", - varop->fullname(), *(static_cast(newDatap)), - *(static_cast(prevDatap)), newDatap, prevDatap);); - if (std::memcmp(prevDatap, newDatap, varop->entSize()) != 0) { + if (valueDiffersFromPrev(varop)) { VL_DEBUG_IF_PLI(VL_DBG_MSGF("- vpi: value_callback %" PRId64 " %s v[0]=%d\n", ho.id(), varop->fullname(), - *(static_cast(newDatap)));); + *(static_cast(varop->varDatap())));); update.insert(varop); vpi_get_value(ho.cb_datap()->obj, ho.cb_datap()->value); (ho.cb_rtnp())(ho.cb_datap()); @@ -1203,9 +1284,7 @@ public: } if (was_last) break; } - for (const VerilatedVpioVar* const ip : update) { - std::memcpy(ip->prevDatap(), ip->varDatap(), ip->entSize()); - } + for (const VerilatedVpioVar* const varop : update) updatePrev(varop); return called; } static void dumpCbs() VL_MT_UNSAFE_ONE; @@ -3195,18 +3274,6 @@ static void vl_strprintf(std::string& buffer, char const* fmt, ...) { va_end(args); } -// Information about how to access packed array data. -// If underlying type is multi-word (VLVT_WDATA), the packed element might straddle word -// boundaries, in which case m_maskHi != 0. -template -struct VarAccessInfo final { - T* m_datap; // Typed pointer to packed array base address - size_t m_bitOffset; // Data start location (bit offset) - size_t m_wordOffset; // Data start location (word offset, VLVT_WDATA only) - T m_maskLo; // Access mask for m_datap[m_wordOffset] - T m_maskHi; // Access mask for m_datap[m_wordOffset + 1] (VLVT_WDATA only) -}; - template VarAccessInfo vl_vpi_var_access_info(const VerilatedVpioVarBase* vop, size_t bitCount, size_t addOffset) { @@ -3262,21 +3329,25 @@ VarAccessInfo vl_vpi_var_access_info(const VerilatedVpioVarBase* vop, size_t } template -T vl_vpi_get_word_gen(const VerilatedVpioVarBase* vop, size_t bitCount, size_t addOffset) { +T vl_vpi_get_word_gen(VarAccessInfo info) { const size_t wordBits = sizeof(T) * 8; - const VarAccessInfo info = vl_vpi_var_access_info(vop, bitCount, addOffset); - if (info.m_maskHi) + if (info.m_maskHi) { return ((info.m_datap[info.m_wordOffset] & info.m_maskLo) >> info.m_bitOffset) | ((info.m_datap[info.m_wordOffset + 1] & info.m_maskHi) << (wordBits - info.m_bitOffset)); + } return (info.m_datap[info.m_wordOffset] & info.m_maskLo) >> info.m_bitOffset; } template -void vl_vpi_put_word_gen(const VerilatedVpioVar* vop, T word, size_t bitCount, size_t addOffset) { - const size_t wordBits = sizeof(T) * 8; +T vl_vpi_get_word_gen(const VerilatedVpioVarBase* vop, size_t bitCount, size_t addOffset) { const VarAccessInfo info = vl_vpi_var_access_info(vop, bitCount, addOffset); + return vl_vpi_get_word_gen(info); +} +template +void vl_vpi_put_word_gen(VarAccessInfo info, T word) { + const size_t wordBits = sizeof(T) * 8; if (info.m_maskHi) { info.m_datap[info.m_wordOffset + 1] = (info.m_datap[info.m_wordOffset + 1] & ~info.m_maskHi) @@ -3287,6 +3358,12 @@ void vl_vpi_put_word_gen(const VerilatedVpioVar* vop, T word, size_t bitCount, s | ((word << info.m_bitOffset) & info.m_maskLo); } +template +void vl_vpi_put_word_gen(const VerilatedVpioVar* vop, T word, size_t bitCount, size_t addOffset) { + const VarAccessInfo info = vl_vpi_var_access_info(vop, bitCount, addOffset); + vl_vpi_put_word_gen(info, word); +} + // bitCount: maximum number of bits to read, will stop earlier if it reaches the var bounds // addOffset: additional read bitoffset QData vl_vpi_get_word(const VerilatedVpioVarBase* vop, size_t bitCount, size_t addOffset) { diff --git a/test_regress/t/t_vpi_var.cpp b/test_regress/t/t_vpi_var.cpp index 9d3e4c3a9..99fa00b2d 100644 --- a/test_regress/t/t_vpi_var.cpp +++ b/test_regress/t/t_vpi_var.cpp @@ -66,6 +66,8 @@ unsigned int callback_count_half = 0; unsigned int callback_count_quad = 0; unsigned int callback_count_strs = 0; unsigned int callback_count_strs_max = 500; +unsigned int callback_count_3d = 0; +unsigned int callback_count_endian = 0; //====================================================================== @@ -237,6 +239,242 @@ int _mon_check_value_callbacks() { return 0; } +int _value_callback_3d(p_cb_data cb_data) { + ++callback_count_3d; + return 0; +} + +int _value_callback_big_endian(p_cb_data cb_data) { + static bool called = false; + if (called) { + printf("%%Error: callback should be called only once\n"); + exit(-1); + } + called = true; + ++callback_count_endian; + return 0; +} + +int _value_callback_little_endian(p_cb_data cb_data) { + static bool called = false; + if (called) { + printf("%%Error: callback should be called only once\n"); + exit(-1); + } + called = true; + ++callback_count_endian; + return 0; +} + +int _mon_check_value_callbacks_array() { + t_cb_data cb_data{}; + cb_data.reason = cbValueChange; + cb_data.time = nullptr; + + { + TestVpiHandle vh = VPI_HANDLE("multi_packed_bit"); + CHECK_RESULT_NZ(vh); + s_vpi_value put_val; + put_val.format = vpiIntVal; + put_val.value.integer = 0x123456; + s_vpi_time time_s = {vpiSimTime, 0, 0, 0.0}; + vpi_put_value(vh, &put_val, &time_s, vpiNoDelay); + } + + { + TestVpiHandle vh1 = VPI_HANDLE("multi_packed_bit[1][2][0]"); + CHECK_RESULT_NZ(vh1); + cb_data.obj = vh1; + cb_data.cb_rtn = _value_callback_never; + + TestVpiHandle callback_h = vpi_register_cb(&cb_data); + CHECK_RESULT_NZ(callback_h); + } + { + // Put value between other values and check if value change applies only here + TestVpiHandle vh1 = VPI_HANDLE("multi_packed_bit[1][2][1]"); + CHECK_RESULT_NZ(vh1); + cb_data.obj = vh1; + cb_data.cb_rtn = _value_callback_3d; + + TestVpiHandle callback_h = vpi_register_cb(&cb_data); + CHECK_RESULT_NZ(callback_h); + + s_vpi_value put_val; + put_val.format = vpiIntVal; + put_val.value.integer = 0; + s_vpi_time time_s = {vpiSimTime, 0, 0, 0.0}; + vpi_put_value(vh1, &put_val, &time_s, vpiNoDelay); + } + { + TestVpiHandle vh1 = VPI_HANDLE("multi_packed_bit[1][2][2]"); + CHECK_RESULT_NZ(vh1); + cb_data.obj = vh1; + cb_data.cb_rtn = _value_callback_never; + + TestVpiHandle callback_h = vpi_register_cb(&cb_data); + CHECK_RESULT_NZ(callback_h); + } + { + TestVpiHandle vh1 = VPI_HANDLE("multi_packed_bit[1][2]"); + CHECK_RESULT_NZ(vh1); + cb_data.obj = vh1; + cb_data.cb_rtn = _value_callback_never; + + TestVpiHandle callback_h = vpi_register_cb(&cb_data); + CHECK_RESULT_NZ(callback_h); + } + { + TestVpiHandle vh1 = VPI_HANDLE("multi_packed_bit[0][0][0]"); + CHECK_RESULT_NZ(vh1); + cb_data.obj = vh1; + cb_data.cb_rtn = _value_callback_never; + + TestVpiHandle callback_h = vpi_register_cb(&cb_data); + CHECK_RESULT_NZ(callback_h); + } + { + TestVpiHandle vh1 = VPI_HANDLE("multi_packed_bit[0][0]"); + CHECK_RESULT_NZ(vh1); + cb_data.obj = vh1; + cb_data.cb_rtn = _value_callback_never; + + TestVpiHandle callback_h = vpi_register_cb(&cb_data); + CHECK_RESULT_NZ(callback_h); + } + { // Set a single bit and hit it with value change, big endian + TestVpiHandle vh = VPI_HANDLE("multi_packed_endian"); + CHECK_RESULT_NZ(vh); + s_vpi_value put_val; + put_val.format = vpiIntVal; + put_val.value.integer = 0b000100; + s_vpi_time time_s = {vpiSimTime, 0, 0, 0.0}; + vpi_put_value(vh, &put_val, &time_s, vpiNoDelay); + + for (int i = -1; i <= 1; ++i) { + TestVpiHandle i_handle = vpi_handle_by_index(vh, i); + CHECK_RESULT_NZ(i_handle); + for (int j = 1; j <= 2; ++j) { + TestVpiHandle i_j_handle = vpi_handle_by_index(i_handle, j); + CHECK_RESULT_NZ(i_j_handle); + cb_data.obj = i_j_handle; + if (i == 0 && j == 1) { + cb_data.cb_rtn = _value_callback_big_endian; + } else { + cb_data.cb_rtn = _value_callback_never; + } + TestVpiHandle callback_h = vpi_register_cb(&cb_data); + CHECK_RESULT_NZ(callback_h); + + s_vpi_value put_val; + put_val.format = vpiIntVal; + put_val.value.integer = 0; + s_vpi_time time_s = {vpiSimTime, 0, 0, 0.0}; + vpi_put_value(i_j_handle, &put_val, &time_s, vpiNoDelay); + } + } + } + { // Set a single bit and hit it with value change, little endian + TestVpiHandle vh = VPI_HANDLE("multi_packed_little_endian"); + CHECK_RESULT_NZ(vh); + s_vpi_value put_val; + put_val.format = vpiIntVal; + put_val.value.integer = 0b001000; + s_vpi_time time_s = {vpiSimTime, 0, 0, 0.0}; + vpi_put_value(vh, &put_val, &time_s, vpiNoDelay); + + for (int i = -1; i <= 1; ++i) { + TestVpiHandle i_handle = vpi_handle_by_index(vh, i); + CHECK_RESULT_NZ(i_handle); + for (int j = 1; j <= 2; ++j) { + TestVpiHandle i_j_handle = vpi_handle_by_index(i_handle, j); + CHECK_RESULT_NZ(i_j_handle); + cb_data.obj = i_j_handle; + if (i == 0 && j == 1) { + cb_data.cb_rtn = _value_callback_little_endian; + } else { + cb_data.cb_rtn = _value_callback_never; + } + TestVpiHandle callback_h = vpi_register_cb(&cb_data); + CHECK_RESULT_NZ(callback_h); + + s_vpi_value put_val; + put_val.format = vpiIntVal; + put_val.value.integer = 0; + s_vpi_time time_s = {vpiSimTime, 0, 0, 0.0}; + vpi_put_value(i_j_handle, &put_val, &time_s, vpiNoDelay); + } + } + } + { + TestVpiHandle vh1 = VPI_HANDLE("multi_unpacked_bit[1][2][3]"); + CHECK_RESULT_NZ(vh1); + cb_data.obj = vh1; + cb_data.cb_rtn = _value_callback_never; + + TestVpiHandle callback_h = vpi_register_cb(&cb_data); + CHECK_RESULT_NZ(callback_h); + } + { + TestVpiHandle vh1 = VPI_HANDLE("multi_unpacked_bit[1][2]"); + CHECK_RESULT_NZ(vh1); + cb_data.obj = vh1; + cb_data.cb_rtn = _value_callback_never; + + TestVpiHandle callback_h = vpi_register_cb(&cb_data); + CHECK_RESULT_NZ(callback_h); + } + { + TestVpiHandle vh1 = VPI_HANDLE("multi_packed_short[1][2]"); + CHECK_RESULT_NZ(vh1); + cb_data.obj = vh1; + cb_data.cb_rtn = _value_callback_never; + + TestVpiHandle callback_h = vpi_register_cb(&cb_data); + CHECK_RESULT_NZ(callback_h); + } + { + TestVpiHandle vh1 = VPI_HANDLE("multi_packed_short[1][0]"); + CHECK_RESULT_NZ(vh1); + cb_data.obj = vh1; + cb_data.cb_rtn = _value_callback_3d; + + TestVpiHandle callback_h = vpi_register_cb(&cb_data); + CHECK_RESULT_NZ(callback_h); + + s_vpi_value put_val; + put_val.format = vpiIntVal; + put_val.value.integer = 1; + s_vpi_time time_s = {vpiSimTime, 0, 0, 0.0}; + vpi_put_value(vh1, &put_val, &time_s, vpiNoDelay); + } + { + TestVpiHandle vh1 = VPI_HANDLE("multi_packed_wide[1][2]"); + CHECK_RESULT_NZ(vh1); + cb_data.obj = vh1; + cb_data.cb_rtn = _value_callback_never; + + TestVpiHandle callback_h = vpi_register_cb(&cb_data); + CHECK_RESULT_NZ(callback_h); + } + { + TestVpiHandle vh1 = VPI_HANDLE("multi_packed_wide[1][0]"); + CHECK_RESULT_NZ(vh1); + cb_data.obj = vh1; + cb_data.cb_rtn = _value_callback_3d; + + TestVpiHandle callback_h = vpi_register_cb(&cb_data); + CHECK_RESULT_NZ(callback_h); + + s_vpi_value put_val; + put_val.format = vpiIntVal; + put_val.value.integer = 1; + s_vpi_time time_s = {vpiSimTime, 0, 0, 0.0}; + vpi_put_value(vh1, &put_val, &time_s, vpiNoDelay); + } + return 0; +} + int _mon_check_big() { #ifdef VERILATOR s_vpi_value v; @@ -2209,6 +2447,7 @@ extern "C" int mon_check() { if (int status = _mon_check_mcd()) return status; if (int status = _mon_check_callbacks()) return status; if (int status = _mon_check_value_callbacks()) return status; + if (int status = _mon_check_value_callbacks_array()) return status; if (int status = _mon_check_var()) return status; if (int status = _mon_check_rev()) return status; if (int status = _mon_check_varlist()) return status; @@ -2311,6 +2550,8 @@ int main(int argc, char** argv) { CHECK_RESULT(callback_count_half, 250); CHECK_RESULT(callback_count_quad, 2); CHECK_RESULT(callback_count_strs, callback_count_strs_max); + CHECK_RESULT(callback_count_3d, 3); + CHECK_RESULT(callback_count_endian, 2); VerilatedVpi::clearEvalNeeded(); if (VerilatedVpi::evalNeeded()) { vl_fatal(FILENM, __LINE__, "main", "%Error: Unexpected VPI dirty state"); diff --git a/test_regress/t/t_vpi_var.v b/test_regress/t/t_vpi_var.v index 5efca1a67..ca2a664d9 100644 --- a/test_regress/t/t_vpi_var.v +++ b/test_regress/t/t_vpi_var.v @@ -52,6 +52,12 @@ extern "C" int mon_check(); reg [0:95] mem_3d[0:1][1:0][0:1] /*verilator public_flat_rw */; // Mixed: asc, desc, asc reg [0:15][0:3][7:0] multi_packed[2:0] /*verilator public_flat_rw */; + reg [0:1][0:3][0:2] multi_packed_bit /*verilator public_flat_rw */; + reg [1:-1][2:1] multi_packed_endian /*verilator public_flat_rw */; + reg [-1:1][1:2] multi_packed_little_endian /*verilator public_flat_rw */; + reg [0:1][0:7] multi_packed_short /*verilator public_flat_rw */; + reg [0:1][0:127] multi_packed_wide /*verilator public_flat_rw */; + reg multi_unpacked_bit[0:1][0:3][0:7] /*verilator public_flat_rw */; reg [8:-7] [3:-4] negative_multi_packed[0:-2] /*verilator public_flat_rw */; // verilator lint_on ASCRANGE reg unpacked_only[7:0]; diff --git a/test_regress/t/t_vpi_var2.v b/test_regress/t/t_vpi_var2.v index 1cd269da7..93082ee8e 100644 --- a/test_regress/t/t_vpi_var2.v +++ b/test_regress/t/t_vpi_var2.v @@ -66,6 +66,12 @@ extern "C" int mon_check(); // Signal with multiple packed dimensions reg [0:15][0:3][7:0] multi_packed[2:0]; + reg [0:1][0:3][0:2] multi_packed_bit /*verilator public_flat_rw */; + reg [1:-1][2:1] multi_packed_endian /*verilator public_flat_rw */; + reg [-1:1][1:2] multi_packed_little_endian /*verilator public_flat_rw */; + reg [0:1][0:7] multi_packed_short /*verilator public_flat_rw */; + reg [0:1][0:127] multi_packed_wide /*verilator public_flat_rw */; + reg multi_unpacked_bit[0:1][0:3][0:7] /*verilator public_flat_rw */; reg [8:-7] [3:-4] negative_multi_packed[0:-2]; // verilator lint_on ASCRANGE reg unpacked_only[7:0]; diff --git a/test_regress/t/t_vpi_var3.v b/test_regress/t/t_vpi_var3.v index e33ba56fb..c82d35859 100644 --- a/test_regress/t/t_vpi_var3.v +++ b/test_regress/t/t_vpi_var3.v @@ -51,6 +51,12 @@ extern "C" int mon_check(); // Signal with multiple packed dimensions reg [0:15][0:3][7:0] multi_packed[2:0]; + reg [0:1][0:3][0:2] multi_packed_bit /*verilator public_flat_rw */; + reg [1:-1][2:1] multi_packed_endian /*verilator public_flat_rw */; + reg [-1:1][1:2] multi_packed_little_endian /*verilator public_flat_rw */; + reg [0:1][0:7] multi_packed_short /*verilator public_flat_rw */; + reg [0:1][0:127] multi_packed_wide /*verilator public_flat_rw */; + reg multi_unpacked_bit[0:1][0:3][0:7] /*verilator public_flat_rw */; reg [8:-7] [3:-4] negative_multi_packed[0:-2]; // verilator lint_on ASCRANGE reg unpacked_only[7:0];