Skip to content

Commit

Permalink
[ET-VK][ez] Fix undefined behaviour in ambiguous ParamsBuffer const…
Browse files Browse the repository at this point in the history
…ructor (#7482)

## Context

I discovered this bug when trying to execute the `vulkan_compute_api_test` binary on Windows. Almost all the tests were failing, with compute shaders producing incorrect results. After bisecting the change, it turns out the culprit is #7015. The diff introduced an alternative templated constructor for `ParamsBuffer` which would initialize an empty UBO with a specified size instead of wrapping a pre-existing object.

The issue is that these constructors are ambiguous because they both are template constructors and both only accept one argument. Therefore, the original constructor would be called when certain callsites intended to call the new constructor. This results in a UBO being created with an incorrect size, and resulted in the tensor's metadata being passed incorrectly into a compute shader.

To fix, I added a dummy argument into the new constructor for disambiguation purposes. I also changed it so that it's not templated, since there's no reason for it to be templated.

Differential Revision: [D67770791](https://our.internmc.facebook.com/intern/diff/D67770791/)

ghstack-source-id: 260031108
Pull Request resolved: #7478

Co-authored-by: Stephen Jia <ssjia@meta.com>
  • Loading branch information
pytorchbot and SS-JIA authored Jan 3, 2025
1 parent 2600cc8 commit 8306cb5
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 6 deletions.
5 changes: 3 additions & 2 deletions backends/vulkan/runtime/api/containers/ParamsBuffer.h
Original file line number Diff line number Diff line change
Expand Up @@ -31,8 +31,9 @@ class ParamsBuffer final {
vulkan_buffer_(
context_p_->adapter_ptr()->vma().create_params_buffer(block)) {}

template <typename Block>
ParamsBuffer(Context* context_p, const VkDeviceSize nbytes)
// The last bool argument, though unused, is required to disambiguate this
// constructor from the one above.
ParamsBuffer(Context* context_p, const VkDeviceSize nbytes, const bool unused)
: context_p_(context_p),
vulkan_buffer_(
context_p_->adapter_ptr()->vma().create_uniform_buffer(nbytes)) {}
Expand Down
8 changes: 4 additions & 4 deletions backends/vulkan/runtime/api/containers/Tensor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -659,7 +659,7 @@ utils::GPUMemoryLayout vTensor::estimate_memory_layout() const {

const vkapi::BufferBindInfo vTensor::sizes_ubo() {
if (!uniforms_.buffer()) {
uniforms_ = ParamsBuffer(storage_.context_, kMaxUniformBufferSize);
uniforms_ = ParamsBuffer(storage_.context_, kMaxUniformBufferSize, true);
}
if (sizes_uniform_offset_ == kUniformOffsetUnset) {
VK_CHECK_COND(
Expand All @@ -674,7 +674,7 @@ const vkapi::BufferBindInfo vTensor::sizes_ubo() {

const vkapi::BufferBindInfo vTensor::strides_ubo() {
if (!uniforms_.buffer()) {
uniforms_ = ParamsBuffer(storage_.context_, kMaxUniformBufferSize);
uniforms_ = ParamsBuffer(storage_.context_, kMaxUniformBufferSize, true);
}
if (unsqueezed_strides_offset_ == kUniformOffsetUnset) {
VK_CHECK_COND(
Expand All @@ -691,7 +691,7 @@ const vkapi::BufferBindInfo vTensor::strides_ubo() {

const vkapi::BufferBindInfo vTensor::logical_limits_ubo() {
if (!uniforms_.buffer()) {
uniforms_ = ParamsBuffer(storage_.context_, kMaxUniformBufferSize);
uniforms_ = ParamsBuffer(storage_.context_, kMaxUniformBufferSize, true);
}
if (logical_limits_uniform_offset_ == kUniformOffsetUnset) {
VK_CHECK_COND(
Expand All @@ -707,7 +707,7 @@ const vkapi::BufferBindInfo vTensor::logical_limits_ubo() {

const vkapi::BufferBindInfo vTensor::numel_ubo() {
if (!uniforms_.buffer()) {
uniforms_ = ParamsBuffer(storage_.context_, kMaxUniformBufferSize);
uniforms_ = ParamsBuffer(storage_.context_, kMaxUniformBufferSize, true);
}
if (numel_uniform_offset_ == kUniformOffsetUnset) {
VK_CHECK_COND(
Expand Down

0 comments on commit 8306cb5

Please sign in to comment.