Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Buffer serialization does not match deserialization #8534

Closed
abadams opened this issue Dec 20, 2024 · 1 comment
Closed

Buffer serialization does not match deserialization #8534

abadams opened this issue Dec 20, 2024 · 1 comment
Assignees
Labels

Comments

@abadams
Copy link
Member

abadams commented Dec 20, 2024

We're currently getting failures for correctness_image_io, but only when the serialization round trip testing is enabled. I think it's likely that serializing buffers is wonky, but I don't know why this would have only just broken now. The serialization code is:

   for (int i = 0; i < buffer.dimensions(); ++i) {
        int32_t min = buffer.dim(i).min();
        int32_t extent = buffer.dim(i).extent();
        int32_t stride = buffer.dim(i).stride();
        buffer_dimensions_serialized.push_back(Serialize::CreateBufferDimension(builder, min, extent, stride));
    }
    auto copy = buffer.copy();  // compact in memory
    std::vector<uint8_t> data;
    data.resize(copy.size_in_bytes());
    memcpy(data.data(), copy.data(), copy.size_in_bytes());

and the deserialization code is:

   for (int i = 0; i < dimensions; ++i) {
        const auto *dim = buffer->dims()->Get(i);
        halide_dimension_t hl_dim, dense_dim;
        hl_dim.min = dim->min();
        hl_dim.extent = dim->extent();
        hl_dim.stride = dim->stride();
        hl_buffer_dimensions.push_back(hl_dim);
        dense_dim.min = hl_dim.min;
        dense_dim.extent = hl_dim.extent;
        if (i == 0) {
            dense_dim.stride = hl_dim.stride;
            stride = hl_dim.stride * hl_dim.extent;
        } else {
            dense_dim.stride = stride;
            stride *= hl_dim.extent;
        }
        dense_buffer_dimensions.push_back(dense_dim);
    }
    // To handle cropped buffer, we create a dense buffer and serialize into it,
    // then create a (potential sparse) buffer with orignal dimension infos and copy from the dense buffer
    auto fake_dense_buffer = Buffer<>(type, nullptr, dimensions, dense_buffer_dimensions.data(), name + "_dense_fake");
    auto dense_buffer = Buffer<>::make_with_shape_of(fake_dense_buffer, nullptr, nullptr, name + "_dense_tmp");
    memcpy(dense_buffer.data(), buffer->data()->data(), buffer->data()->size());
    auto fake_buffer = Buffer<>(type, nullptr, dimensions, hl_buffer_dimensions.data(), name + "_fake");
    auto hl_buffer = Buffer<>::make_with_shape_of(fake_buffer, nullptr, nullptr, name);
    hl_buffer.copy_from(dense_buffer);

The deserialization code appears to assume that the strides are in increasing order, which buffer.copy() does not guarantee (it preserves dimension ordering). I will attempt to fix.

@abadams abadams self-assigned this Dec 20, 2024
@abadams abadams added the bug label Dec 20, 2024
@abadams
Copy link
Member Author

abadams commented Dec 20, 2024

Red herring, the problem was actually due to a clang tidy false positive in #8509

abadams added a commit that referenced this issue Dec 20, 2024
This needs to take src by value, because the helper function mutates
ones of its members in-place.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

1 participant