Fix VPI force of bit-selected signals (#7211) (#7301)

include/verilated_vpi.cpp

@@ -395,6 +395,7 @@ public:
         return dynamic_cast<VerilatedVpioVar*>(reinterpret_cast<VerilatedVpio*>(h));
     }
     uint32_t bitOffset() const override { return m_bitOffset; }
+    int32_t partselBits() const { return m_partselBits; }
     uint32_t bitSize() const {
         if (m_partselBits >= 0) return static_cast<uint32_t>(m_partselBits);
         return VerilatedVpioVarBase::bitSize();
@@ -1119,7 +1120,7 @@ public:
         }
         s().m_inertialPuts.clear();
     }
-    static auto getForceControlSignals(const VerilatedVpioVarBase* vop);
+    static auto getForceControlSignals(const VerilatedVpioVar* vop);
 
     static std::size_t vlTypeSize(VerilatedVarType vltype);
     static void setAllBitsToValue(const VerilatedVpioVar* vop, uint8_t bitValue) {
@@ -1314,15 +1315,16 @@ VerilatedVpiError* VerilatedVpiImp::error_info() VL_MT_UNSAFE_ONE {
     return s().m_errorInfop;
 }
 
-auto VerilatedVpiImp::getForceControlSignals(const VerilatedVpioVarBase* const baseSignalVop) {
+auto VerilatedVpiImp::getForceControlSignals(const VerilatedVpioVar* const baseSignalVop) {
     const VerilatedForceControlSignals* const forceControlSignals
         = baseSignalVop->varp()->forceControlSignals();
     // LCOV_EXCL_START - Would require a Verilation time error, so cannot test
     if (VL_UNLIKELY(!forceControlSignals)) {
-        VL_VPI_ERROR_(__FILE__, __LINE__,
+        VL_VPI_ERROR_(
-                      "%s: VPI force or release requested for '%s', but signal has no force "
+            __FILE__, __LINE__,
-                      "control signals. Ensure signal is marked as forceable",
+            "%s: VPI put or get requested for forceable signal '%s', but signal has no force "
-                      __func__, baseSignalVop->fullname());
+            "control signals.",
+            __func__, baseSignalVop->fullname());
         return std::pair<std::unique_ptr<VerilatedVpioVar>, std::unique_ptr<VerilatedVpioVar>>{
             nullptr, nullptr};
     }  // LCOV_EXCL_STOP
@@ -1331,26 +1333,141 @@ auto VerilatedVpiImp::getForceControlSignals(const VerilatedVpioVarBase* const b
     // LCOV_EXCL_START - Would require a Verilation time error, so cannot test
     if (VL_UNLIKELY(!forceEnableSignalVarp)) {
         VL_VPI_ERROR_(__FILE__, __LINE__,
-                      "%s: VPI force or release requested for '%s', but force enable signal could "
+                      "%s: VPI put or get requested for forceable signal '%s', but force enable "
-                      "not be found. Ensure signal is marked as forceable",
+                      "signal could not be found.",
                       __func__, baseSignalVop->fullname());
         return std::pair<std::unique_ptr<VerilatedVpioVar>, std::unique_ptr<VerilatedVpioVar>>{
             nullptr, nullptr};
     }
     if (VL_UNLIKELY(!forceValueSignalVarp)) {
-        VL_VPI_ERROR_(__FILE__, __LINE__,
+        VL_VPI_ERROR_(
-                      "%s: VPI force or release requested for '%s', but force value signal could "
+            __FILE__, __LINE__,
-                      "not be found. Ensure signal is marked as forceable",
+            "%s: VPI put or get requested for forceable signal '%s', but force value signal could "
-                      __func__, baseSignalVop->fullname());
+            "not be found.",
+            __func__, baseSignalVop->fullname());
         return std::pair<std::unique_ptr<VerilatedVpioVar>, std::unique_ptr<VerilatedVpioVar>>{
             nullptr, nullptr};
     }
     // LCOV_EXCL_STOP
-    VerilatedVpioVar forceEnableSignal{forceEnableSignalVarp, baseSignalVop->scopep()};
+
-    VerilatedVpioVar forceValueSignal{forceValueSignalVarp, baseSignalVop->scopep()};
+    // Adjust dimension and partselect to match the base signal, so that forcing a partial signal
+    // doesn't exceed the bounds given by the base signal
+    VerilatedVpioVar* forceEnableSignalVop
+        = new VerilatedVpioVar{forceEnableSignalVarp, baseSignalVop->scopep()};
+    VerilatedVpioVar* forceValueSignalVop
+        = new VerilatedVpioVar{forceValueSignalVarp, baseSignalVop->scopep()};
+
+    for (int idx : baseSignalVop->index()) {
+        VerilatedVpioVar* nextForceEnableSignalVop = forceEnableSignalVop->withIndex(idx);
+        VerilatedVpioVar* nextForceValueSignalVop = forceValueSignalVop->withIndex(idx);
+        VL_DO_DANGLING(delete forceEnableSignalVop, forceEnableSignalVop);
+        VL_DO_DANGLING(delete forceValueSignalVop, forceValueSignalVop);
+        forceEnableSignalVop = nextForceEnableSignalVop;
+        forceValueSignalVop = nextForceValueSignalVop;
+        if (!forceEnableSignalVop || !forceValueSignalVop) break;  // LCOV_EXCL_LINE
+    }
+
+    // LCOV_EXCL_START - Would require a Verilation time error, so cannot test
+    if (VL_UNLIKELY(!forceEnableSignalVop)) {
+        VL_VPI_ERROR_(__FILE__, __LINE__,
+                      "%s: VPI put or get requested for forceable signal '%s', but force enable "
+                      "signal could not be indexed to the same dimension as the base signal.",
+                      __func__, baseSignalVop->fullname());
+        if (VL_UNLIKELY(forceValueSignalVop))
+            VL_DO_DANGLING(delete forceValueSignalVop, forceValueSignalVop);
+        return std::pair<std::unique_ptr<VerilatedVpioVar>, std::unique_ptr<VerilatedVpioVar>>{
+            nullptr, nullptr};
+    }
+    if (VL_UNLIKELY(!forceValueSignalVop)) {
+        VL_VPI_ERROR_(__FILE__, __LINE__,
+                      "%s: VPI put or get requested for forceable signal '%s', but force value "
+                      "signal could not be indexed to the same dimension as the base signal.",
+                      __func__, baseSignalVop->fullname());
+        if (VL_UNLIKELY(forceEnableSignalVop))
+            VL_DO_DANGLING(delete forceEnableSignalVop, forceEnableSignalVop);
+        return std::pair<std::unique_ptr<VerilatedVpioVar>, std::unique_ptr<VerilatedVpioVar>>{
+            nullptr, nullptr};
+    }
+    // LCOV_EXCL_STOP
+
+    if (VL_UNLIKELY(baseSignalVop->partselBits() != -1)) {
+        // Bits are stored left-to-right in memory, which can either be ascending or descending. To
+        // match the bitOffset of the base signal, the distance to the rightmost bit, rather than
+        // to the lowest indexed bit, must be determined
+        int currentDimRight = baseSignalVop->rangep()->right();
+        int32_t offsetFromRight
+            = static_cast<int32_t>(static_cast<int64_t>(baseSignalVop->bitOffset())
+                                   - static_cast<int64_t>(forceValueSignalVop->bitOffset()));
+        const bool isDescending
+            = baseSignalVop->rangep()->left() >= baseSignalVop->rangep()->right();
+
+        const int32_t partSelIndexRight
+            = isDescending ? currentDimRight + offsetFromRight : currentDimRight - offsetFromRight;
+        const int32_t partSelIndexLeft
+            = isDescending ? partSelIndexRight + (baseSignalVop->partselBits() - 1)
+                           : partSelIndexRight - (baseSignalVop->partselBits() - 1);
+        const int32_t partSelIndexHigh = std::max(partSelIndexLeft, partSelIndexRight);
+        const int32_t partSelIndexLow = std::min(partSelIndexLeft, partSelIndexRight);
+
+        VerilatedVpioVar* partIndexedForceEnableSignalVop
+            = forceEnableSignalVop->withPartSelect(partSelIndexHigh, partSelIndexLow);
+        VerilatedVpioVar* partIndexedForceValueSignalVop
+            = forceValueSignalVop->withPartSelect(partSelIndexHigh, partSelIndexLow);
+
+        VL_DO_DANGLING(delete forceEnableSignalVop, forceEnableSignalVop);
+        VL_DO_DANGLING(delete forceValueSignalVop, forceValueSignalVop);
+        forceEnableSignalVop = partIndexedForceEnableSignalVop;
+        forceValueSignalVop = partIndexedForceValueSignalVop;
+    }
+
+    // LCOV_EXCL_START - Would require a Verilation time error, so cannot test
+    if (VL_UNLIKELY(!forceEnableSignalVop)) {
+        VL_VPI_ERROR_(__FILE__, __LINE__,
+                      "%s: VPI put or get requested for forceable signal '%s', but part selection "
+                      "could not be applied to the force enable signal.",
+                      __func__, baseSignalVop->fullname());
+        if (VL_UNLIKELY(forceValueSignalVop))
+            VL_DO_DANGLING(delete forceValueSignalVop, forceValueSignalVop);
+        return std::pair<std::unique_ptr<VerilatedVpioVar>, std::unique_ptr<VerilatedVpioVar>>{
+            nullptr, nullptr};
+    }
+    if (VL_UNLIKELY(!forceValueSignalVop)) {
+        VL_VPI_ERROR_(__FILE__, __LINE__,
+                      "%s: VPI put or get requested for forceable signal '%s', but part selection "
+                      "could not be applied to the force value signal.",
+                      __func__, baseSignalVop->fullname());
+        if (VL_UNLIKELY(forceEnableSignalVop))
+            VL_DO_DANGLING(delete forceEnableSignalVop, forceEnableSignalVop);
+        return std::pair<std::unique_ptr<VerilatedVpioVar>, std::unique_ptr<VerilatedVpioVar>>{
+            nullptr, nullptr};
+    }
+    // LCOV_EXCL_STOP
+
+#ifdef VL_DEBUG
+    // Sanity checks: Offsets, widths, and dimensions should all match between the base signal and
+    // the force control signals, so that they always refer to the same bits
+    assert(forceEnableSignalVop->bitSize() == baseSignalVop->bitSize());
+    assert(forceValueSignalVop->bitSize() == baseSignalVop->bitSize());
+    assert(forceEnableSignalVop->indexedDim() == baseSignalVop->indexedDim());
+    assert(forceValueSignalVop->indexedDim() == baseSignalVop->indexedDim());
+    assert(forceEnableSignalVop->index() == baseSignalVop->index());
+    assert(forceValueSignalVop->index() == baseSignalVop->index());
+    assert(forceEnableSignalVop->bitOffset() == baseSignalVop->bitOffset());
+    assert(forceValueSignalVop->bitOffset() == baseSignalVop->bitOffset());
+    assert(forceEnableSignalVop->partselBits() == baseSignalVop->partselBits());
+    assert(forceValueSignalVop->partselBits() == baseSignalVop->partselBits());
+    // entSize can differ because the force enable signal can have a different vltyp than the base
+    // signal (e.g. the base signal can be of type VLVT_REAL, while the force enable signal is
+    // still of type VLVT_UINT8), but for now entSize is only used for unpacked arrays, which
+    // cannot be forced through VPI yet
+    // assert(forceEnableSignalVop->entSize() == baseSignalVop->entSize());
+    assert(forceValueSignalVop->entSize() == baseSignalVop->entSize());
+    assert(forceEnableSignalVop->varDatap() == forceEnableSignalVarp->datap());
+    assert(forceValueSignalVop->varDatap() == forceValueSignalVarp->datap());
+#endif  // VL_DEBUG
+
     return std::pair<std::unique_ptr<VerilatedVpioVar>, std::unique_ptr<VerilatedVpioVar>>{
-        std::make_unique<VerilatedVpioVar>(forceEnableSignal),
+        forceEnableSignalVop, forceValueSignalVop};
-        std::make_unique<VerilatedVpioVar>(forceValueSignal)};
 }
 
 std::size_t VerilatedVpiImp::vlTypeSize(const VerilatedVarType vltype) {
@@ -2902,11 +3019,15 @@ void vl_vpi_get_value(const VerilatedVpioVarBase* vop, p_vpi_value valuep) {
     // __VforceRd already has the correct value, but that signal is not public and thus not present
     // in the scope's m_varsp map, will be removed entirely eventually (#7092), so its value has to
     // be recreated using the __VforceEn and __VforceVal signals.
-    const auto forceControlSignals
+    const auto forceControlSignals = [vop]() {
-        = vop->varp()->isForceable()
+        if (vop->varp()->isForceable()) {
-              ? VerilatedVpiImp::getForceControlSignals(vop)
+            const VerilatedVpioVar* varVop = dynamic_cast<const VerilatedVpioVar*>(vop);
-              : std::pair<std::unique_ptr<VerilatedVpioVar>, std::unique_ptr<VerilatedVpioVar>>{
+            assert(varVop);  // VerilatedVpioParams cannot be forced
-                    nullptr, nullptr};
+            return VerilatedVpiImp::getForceControlSignals(varVop);
+        }
+        return std::pair<std::unique_ptr<VerilatedVpioVar>, std::unique_ptr<VerilatedVpioVar>>{
+            nullptr, nullptr};
+    }();
     const VerilatedVpioVarBase* const forceEnableSignalVop = forceControlSignals.first.get();
     const VerilatedVpioVarBase* const forceValueSignalVop = forceControlSignals.second.get();
     t_vpi_error_info getForceControlSignalsError{};

test_regress/t/t_vpi_var.cpp

@@ -1071,6 +1071,19 @@ int _mon_check_multi_index() {
         vpi_get_value(vh_3d, &v);
         CHECK_RESULT(v.value.integer, 7);  // (1*4) + (1*2) + 1
 
+        // 2D Packed array with negative indices: [8:-7] [3:-4] negative_multi_packed[0:-2]
+        TestVpiHandle vh_neg_packed_base
+            = vpi_handle_by_name((PLI_BYTE8*)"t.negative_multi_packed", nullptr);
+        CHECK_RESULT_NZ(vh_neg_packed_base);
+        PLI_INT32 idx_neg_packed[2] = {-1, -2};
+        TestVpiHandle vh_neg_packed
+            = vpi_handle_by_multi_index(vh_neg_packed_base, 2, idx_neg_packed);
+        CHECK_RESULT_NZ(vh_neg_packed);
+        CHECK_RESULT(vpi_get(vpiType, vh_neg_packed), vpiReg);
+        CHECK_RESULT(vpi_get(vpiSize, vh_neg_packed), 8);
+        vpi_get_value(vh_neg_packed, &v);
+        CHECK_RESULT(v.value.integer, 4);
+
         // Verify multi_index matches sequential vpi_handle_by_index
         TestVpiHandle vh_seq_base = vpi_handle_by_name((PLI_BYTE8*)"t.mem_2d", nullptr);
         CHECK_RESULT_NZ(vh_seq_base);
@@ -1266,6 +1279,15 @@ int _mon_check_multi_index() {
         CHECK_RESULT(vpi_get(vpiSize, vh_3d), 96);
         vpi_get_value(vh_3d, &v);
         CHECK_RESULT(v.value.integer, 7);
+
+        // Index into single bit with negative index
+        TestVpiHandle vh_neg_bit
+            = vpi_handle_by_name((PLI_BYTE8*)"t.negative_multi_packed[-1][-2][-2]", nullptr);
+        CHECK_RESULT_NZ(vh_neg_bit);
+        CHECK_RESULT(vpi_get(vpiSize, vh_neg_bit), 1);
+        vpi_get_value(vh_neg_bit, &v);
+        // Element [-1][-2] is 8'h4; elements are indexed as [3:-4], so bit -2 is 1
+        CHECK_RESULT(v.value.integer, 1);
     }
 
     // Packed dimension indexing: quads[2] bit selection
@@ -1322,6 +1344,24 @@ int _mon_check_multi_index() {
         CHECK_RESULT_NZ(vh_last);
         vpi_get_value(vh_last, &v);
         CHECK_RESULT(v.value.integer, 0xDD);
+
+        // Negative indices: negative_multi_packed is defined as
+        // `[8:-7] [3:-4] negative_multi_packed[0:-2]`
+        TestVpiHandle vh_neg
+            = vpi_handle_by_name((PLI_BYTE8*)"t.negative_multi_packed[-1]", nullptr);
+        CHECK_RESULT_NZ(vh_neg);
+        CHECK_RESULT(vpi_get(vpiSize, vh_neg), 128);
+        TestVpiHandle vh_neg_packed = vpi_handle_by_index(vh_neg, -2);
+        CHECK_RESULT_NZ(vh_neg_packed);
+        CHECK_RESULT(vpi_get(vpiSize, vh_neg_packed), 8);
+        vpi_get_value(vh_neg_packed, &v);
+        CHECK_RESULT(v.value.integer, 4);
+        // Further into bit level
+        TestVpiHandle vh_neg_bit = vpi_handle_by_index(vh_neg_packed, -2);
+        CHECK_RESULT_NZ(vh_neg_bit);
+        CHECK_RESULT(vpi_get(vpiSize, vh_neg_bit), 1);
+        vpi_get_value(vh_neg_bit, &v);
+        CHECK_RESULT(v.value.integer, 1);
     }
 
     // Partial indexing (not all unpacked dimensions)
@@ -1350,6 +1390,8 @@ int _mon_check_multi_index() {
         // Non-integer / non-decimal index values
         CHECK_RESULT_Z(vpi_handle_by_name((PLI_BYTE8*)"t.mem_2d[0][abc]", nullptr));
         CHECK_RESULT_Z(vpi_handle_by_name((PLI_BYTE8*)"t.mem_2d[0x2][3]", nullptr));
+        // Index out of bounds
+        CHECK_RESULT_Z(vpi_handle_by_name((PLI_BYTE8*)"t.mem_2d[4][0]", nullptr));
         CHECK_RESULT_Z(vpi_handle_by_name((PLI_BYTE8*)"t.mem_2d[-1][0]", nullptr));
         // Structural bracket errors
         CHECK_RESULT_Z(vpi_handle_by_name((PLI_BYTE8*)"t.mem_2d[0][]", nullptr));
@@ -1366,7 +1408,8 @@ int _mon_check_multi_index() {
         CHECK_RESULT_Z(vpi_handle_by_name((PLI_BYTE8*)"t.\\escaped_inst[0] .sig [3:0]", nullptr));
         // Indexing non-array signals
         CHECK_RESULT_Z(vpi_handle_by_name((PLI_BYTE8*)"t.onebit[0]", nullptr));
-        CHECK_RESULT_Z(vpi_handle_by_name((PLI_BYTE8*)"t.twoone[0]", nullptr));
+        // Part-select on non-array signal
+        CHECK_RESULT_Z(vpi_handle_by_name((PLI_BYTE8*)"t.onebit[0:0]", nullptr));
         // Part-select on unpacked-only array
         CHECK_RESULT_Z(vpi_handle_by_name((PLI_BYTE8*)"t.unpacked_only[3:0]", nullptr));
         // Range/slice syntax in non-last position or on unpacked dimensions
@@ -1402,6 +1445,15 @@ int _mon_check_multi_index() {
         vpi_get_value(vh_desc_full, &v);
         CHECK_RESULT(v.value.integer, 24);  // 0x18
 
+        // Descending range that crosses zero
+        TestVpiHandle vh_desc_cross
+            = vpi_handle_by_name((PLI_BYTE8*)"t.negative_multi_packed[-1][-2][1:-3]", nullptr);
+        CHECK_RESULT_NZ(vh_desc_cross);
+        CHECK_RESULT(vpi_get(vpiSize, vh_desc_cross), 5);
+        vpi_get_value(vh_desc_cross, &v);
+        // Element [-1][-2] is 8'h4; elements are indexed as [3:-4], so bits [1:-3] = 0b00010
+        CHECK_RESULT(v.value.integer, 2);
+
         // Ascending packed range behavior is explicit:
         // mem_3d has packed declaration [0:95], so [3:0] selects the MSB-end nibble,
         // while [92:95] selects the LSB-end nibble where value 7 resides.

test_regress/t/t_vpi_var.v

@@ -50,6 +50,7 @@ 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 [8:-7] [3:-4] negative_multi_packed[0:-2]  /*verilator public_flat_rw */;
   // verilator lint_on ASCRANGE
   reg             unpacked_only[7:0];
 
@@ -131,6 +132,12 @@ extern "C" int mon_check();
       end
     end
 
+    for (int i = -2; i <= 0; i++) begin
+      for (int j = -7; j <= 8; j++) begin
+        negative_multi_packed[i][j] = 8'(((i + 2) * 4) + (j + 2));
+      end
+    end
+
 `ifdef VERILATOR
     status = $c32("mon_check()");
 `endif

test_regress/t/t_vpi_var2.v

@@ -66,6 +66,7 @@ extern "C" int mon_check();
 
   // Signal with multiple packed dimensions
   reg [0:15][0:3][7:0] multi_packed[2:0];
+  reg [8:-7] [3:-4] negative_multi_packed[0:-2];
   // verilator lint_on ASCRANGE
   reg             unpacked_only[7:0];
 /*verilator public_off*/
@@ -149,6 +150,12 @@ extern "C" int mon_check();
       end
     end
 
+    for (int i = -2; i <= 0; i++) begin
+      for (int j = -7; j <= 8; j++) begin
+        negative_multi_packed[i][j] = 8'(((i + 2) * 4) + (j + 2));
+      end
+    end
+
 `ifdef VERILATOR
     status = $c32("mon_check()");
 `endif

test_regress/t/t_vpi_var3.v

@@ -51,6 +51,7 @@ extern "C" int mon_check();
 
   // Signal with multiple packed dimensions
   reg [0:15][0:3][7:0] multi_packed[2:0];
+  reg [8:-7] [3:-4] negative_multi_packed[0:-2];
   // verilator lint_on ASCRANGE
   reg             unpacked_only[7:0];
 
@@ -129,6 +130,12 @@ extern "C" int mon_check();
       end
     end
 
+    for (int i = -2; i <= 0; i++) begin
+      for (int j = -7; j <= 8; j++) begin
+        negative_multi_packed[i][j] = 8'(((i + 2) * 4) + (j + 2));
+      end
+    end
+
 `ifdef VERILATOR
     status = $c32("mon_check()");
 `endif