Skip to content

Commit ed7e765

Browse files
committed
[ET-VK][ez] Fix using a temporary variable when creating ComputePipeline
## Context Fix changes introduced in #8634 (D70021032). This change decoupled local work group size from additional specialization constants. As part of this change, when creating a `VkComputePipeline` a temporary `SpecVarList` is created to merge the `WorkgroupSize` with additional specialization constants. However, this can be an issue with some Vulkan drivers because the `SpecVarList` is a temporary, and thus will be destroyed at the end of the function call. The pointer stored in the `VkSpecializationInfo` will be invalidated, leading to undefined behaviour. This diff fixes this by restoring the behaviour of `ComputePipeline::Descriptor` storing the `specialization_constants` directly. Also fix the fact that the `VkSpecializationMapEntry` vector was also a temporary when creating a `VkComputePipeline` by storing it in `ComputePipeline`. Differential Revision: [D71488015](https://our.internmc.facebook.com/intern/diff/D71488015/) [ghstack-poisoned]
1 parent 6daff83 commit ed7e765

File tree

3 files changed

+17
-22
lines changed

3 files changed

+17
-22
lines changed

backends/vulkan/runtime/api/Context.cpp

+8-2
Original file line numberDiff line numberDiff line change
@@ -124,11 +124,17 @@ vkapi::DescriptorSet Context::get_descriptor_set(
124124
VkPipelineLayout pipeline_layout =
125125
pipeline_layout_cache().retrieve(shader_layout, push_constants_size);
126126

127+
vkapi::SpecVarList spec_constants = {
128+
SV(local_workgroup_size[0u]),
129+
SV(local_workgroup_size[1u]),
130+
SV(local_workgroup_size[2u])};
131+
132+
spec_constants.append(additional_constants);
133+
127134
VkPipeline pipeline = pipeline_cache().retrieve(
128135
{pipeline_layout_cache().retrieve(shader_layout, push_constants_size),
129136
shader_cache().retrieve(shader_descriptor),
130-
additional_constants,
131-
local_workgroup_size});
137+
spec_constants});
132138

133139
cmd_.bind_pipeline(pipeline, pipeline_layout, local_workgroup_size);
134140

backends/vulkan/runtime/vk_api/Pipeline.cpp

+8-16
Original file line numberDiff line numberDiff line change
@@ -275,23 +275,13 @@ ComputePipeline::ComputePipeline(
275275
const ComputePipeline::Descriptor& descriptor,
276276
VkPipelineCache pipeline_cache)
277277
: device_(device), handle_{VK_NULL_HANDLE} {
278-
SpecVarList specialization_constants;
279-
280-
specialization_constants.reserve(
281-
3 + descriptor.specialization_constants.size());
282-
specialization_constants.append(descriptor.local_wg_size[0]);
283-
specialization_constants.append(descriptor.local_wg_size[1]);
284-
specialization_constants.append(descriptor.local_wg_size[2]);
285-
286-
specialization_constants.append(descriptor.specialization_constants);
287-
const std::vector<VkSpecializationMapEntry> map_entries =
288-
specialization_constants.generate_map_entries();
278+
map_entries_ = descriptor.specialization_constants.generate_map_entries();
289279

290280
const VkSpecializationInfo specialization_info{
291-
specialization_constants.size(), // mapEntryCount
292-
map_entries.data(), // pMapEntries
293-
specialization_constants.data_nbytes(), // dataSize
294-
specialization_constants.data(), // pData
281+
descriptor.specialization_constants.size(), // mapEntryCount
282+
map_entries_.data(), // pMapEntries
283+
descriptor.specialization_constants.data_nbytes(), // dataSize
284+
descriptor.specialization_constants.data(), // pData
295285
};
296286

297287
const VkPipelineShaderStageCreateInfo shader_stage_create_info{
@@ -330,7 +320,9 @@ ComputePipeline::ComputePipeline(
330320
}
331321

332322
ComputePipeline::ComputePipeline(ComputePipeline&& other) noexcept
333-
: device_(other.device_), handle_(other.handle_) {
323+
: device_(other.device_),
324+
handle_(other.handle_),
325+
map_entries_(std::move(other.map_entries_)) {
334326
other.handle_ = VK_NULL_HANDLE;
335327
}
336328

backends/vulkan/runtime/vk_api/Pipeline.h

+1-4
Original file line numberDiff line numberDiff line change
@@ -156,7 +156,6 @@ class ComputePipeline final {
156156
VkPipelineLayout pipeline_layout;
157157
VkShaderModule shader_module;
158158
SpecVarList specialization_constants;
159-
utils::WorkgroupSize local_wg_size;
160159
};
161160

162161
explicit ComputePipeline(
@@ -175,6 +174,7 @@ class ComputePipeline final {
175174
private:
176175
VkDevice device_;
177176
VkPipeline handle_;
177+
std::vector<VkSpecializationMapEntry> map_entries_;
178178

179179
public:
180180
inline VkPipeline handle() const {
@@ -274,9 +274,6 @@ class ComputePipelineCache final {
274274
seed = utils::hash_combine(seed, new_seed);
275275
}
276276

277-
seed = utils::hash_combine(
278-
seed, std::hash<uint32_t>()((uint32_t)descriptor.local_wg_size));
279-
280277
return seed;
281278
}
282279
};

0 commit comments

Comments
 (0)