Skip to content

Commit 1424286

Browse files
authored
[FFI] Make StructuralEqual functor compare tensor content (#646)
Fixes #645. The `StructuralEqual` functor calls `Equal(lhs, rhs, false, /*skip_tensor_content=*/true)`, while the `StructuralHash` functor hashes content (`skip_tensor_content = false`). The two are used together as the hash and key-equal of the constant de-duplication map in Relax VM codegen (`const_dedup_map_` in apache/tvm `src/relax/backend/vm/exec_builder.cc`). When hash and equal disagree, the map invariant `equal(a, b) => hash(a) == hash(b)` does not hold, so two distinct constants of equal shape and dtype get merged on a bucket collision and a later op reads the wrong constant. Which pair collides depends on the STL bucket count, so the same model produces wrong output under MSVC and correct output under libstdc++. The defect is latent on every platform. This change makes the functor compare content, so it agrees with the hash. Constants that are genuinely equal are still de-duplicated. Verified on a Windows build (MSVC 19.44, LLVM 18.1.8): after the change, YOLO11n det, YOLO11s cls, PP-OCRv5 det and PP-OCRv5 rec all match onnxruntime to within floating point; before, det and the PP-OCR models were off by 6 to 61 percent relative error. Full analysis in #645.
1 parent 88e066e commit 1424286

2 files changed

Lines changed: 44 additions & 1 deletion

File tree

include/tvm/ffi/extra/structural_equal.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ class StructuralEqual {
6969
* \return True if the two Any values are structurally equal, false otherwise.
7070
*/
7171
TVM_FFI_INLINE bool operator()(const Any& lhs, const Any& rhs) const {
72-
return Equal(lhs, rhs, false, true);
72+
return Equal(lhs, rhs, false, false);
7373
}
7474
};
7575

tests/cpp/extra/test_structural_equal_hash.cc

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
#include <tvm/ffi/container/dict.h>
2323
#include <tvm/ffi/container/list.h>
2424
#include <tvm/ffi/container/map.h>
25+
#include <tvm/ffi/container/tensor.h>
2526
#include <tvm/ffi/extra/structural_equal.h>
2627
#include <tvm/ffi/extra/structural_hash.h>
2728
#include <tvm/ffi/object.h>
@@ -36,6 +37,24 @@ using namespace tvm::ffi;
3637
using namespace tvm::ffi::testing;
3738
namespace refl = tvm::ffi::reflection;
3839

40+
// CPU allocator and a helper to build a 1-D float32 tensor filled with one
41+
// value, like test_tensor.cc does.
42+
struct CPUNDAlloc {
43+
void AllocData(DLTensor* tensor) { tensor->data = malloc(GetDataSize(*tensor)); }
44+
void FreeData(DLTensor* tensor) { free(tensor->data); }
45+
};
46+
47+
Tensor MakeFilledTensor(const Shape& shape, float value) {
48+
Tensor t = Tensor::FromNDAlloc(CPUNDAlloc(), shape, DLDataType({kDLFloat, 32, 1}),
49+
DLDevice({kDLCPU, 0}));
50+
float* dst = reinterpret_cast<float*>(t.data_ptr());
51+
// dst points at GetDataSize(t) bytes (numel floats); the analyzer cannot infer that
52+
// size through the allocator, so it wrongly flags this write as out of bounds.
53+
// NOLINTNEXTLINE(clang-analyzer-security.ArrayBound)
54+
for (int64_t i = 0; i < t.numel(); ++i) dst[i] = value;
55+
return t;
56+
}
57+
3958
TEST(StructuralEqualHash, Array) {
4059
Array<int> a = {1, 2, 3};
4160
Array<int> b = {1, 2, 3};
@@ -333,4 +352,28 @@ TEST(StructuralEqualHash, ArraySelfInsertProducesSnapshot) {
333352
EXPECT_EQ(StructuralHash()(arr), StructuralHash()(arr));
334353
}
335354

355+
// Regression test for #645. StructuralHash hashes tensor content, so the
356+
// StructuralEqual functor has to compare content too. Otherwise two distinct
357+
// same-shape constants hash differently but compare equal, which breaks the
358+
// constant de-dup map invariant and can silently merge different weights on a
359+
// bucket collision.
360+
TEST(StructuralEqualHash, TensorContent) {
361+
Tensor zeros = MakeFilledTensor({4}, 0.0f);
362+
Tensor ones = MakeFilledTensor({4}, 1.0f);
363+
Tensor zeros_copy = MakeFilledTensor({4}, 0.0f);
364+
365+
// Different content, same shape and dtype: not equal, and the hash differs.
366+
EXPECT_FALSE(StructuralEqual()(zeros, ones));
367+
EXPECT_NE(StructuralHash()(zeros), StructuralHash()(ones));
368+
369+
// Identical content still compares equal and hashes equal, so real duplicates
370+
// still get merged.
371+
EXPECT_TRUE(StructuralEqual()(zeros, zeros_copy));
372+
EXPECT_EQ(StructuralHash()(zeros), StructuralHash()(zeros_copy));
373+
374+
// Skipping content is still available as an explicit opt-in.
375+
EXPECT_TRUE(StructuralEqual::Equal(zeros, ones, /*map_free_vars=*/false,
376+
/*skip_tensor_content=*/true));
377+
}
378+
336379
} // namespace

0 commit comments

Comments
 (0)