From bc1728065b5d93cc415ed76a51e695fda34c2542 Mon Sep 17 00:00:00 2001
From: Jim Blandy <jimb@red-bean.com>
Date: Wed, 9 Nov 2022 12:50:06 -0800
Subject: [PATCH] wgpu-hal: Document some details of buffer binding. (#3169)

---
 wgpu-hal/src/lib.rs          | 19 ++++++++++++++++++
 wgpu-hal/src/metal/device.rs | 12 +++++++++++
 wgpu-hal/src/metal/mod.rs    | 39 ++++++++++++++++++++++++++++++++++++
 3 files changed, 70 insertions(+)

diff --git a/wgpu-hal/src/lib.rs b/wgpu-hal/src/lib.rs
index 1e3c78caee..9dc59dc9f8 100644
--- a/wgpu-hal/src/lib.rs
+++ b/wgpu-hal/src/lib.rs
@@ -884,8 +884,27 @@ pub struct PipelineLayoutDescriptor<'a, A: Api> {
 
 #[derive(Debug)]
 pub struct BufferBinding<'a, A: Api> {
+    /// The buffer being bound.
     pub buffer: &'a A::Buffer,
+
+    /// The offset at which the bound region starts.
+    ///
+    /// This must be less than the size of the buffer. Some back ends
+    /// cannot tolerate zero-length regions; for example, see
+    /// [VUID-VkDescriptorBufferInfo-offset-00340][340] and
+    /// [VUID-VkDescriptorBufferInfo-range-00341][341], or the
+    /// documentation for GLES's [glBindBufferRange][bbr].
+    ///
+    /// [340]: https://registry.khronos.org/vulkan/specs/1.3-extensions/html/vkspec.html#VUID-VkDescriptorBufferInfo-offset-00340
+    /// [341]: https://registry.khronos.org/vulkan/specs/1.3-extensions/html/vkspec.html#VUID-VkDescriptorBufferInfo-range-00341
+    /// [bbr]: https://registry.khronos.org/OpenGL-Refpages/es3.0/html/glBindBufferRange.xhtml
     pub offset: wgt::BufferAddress,
+
+    /// The size of the region bound, in bytes.
+    ///
+    /// If `None`, the region extends from `offset` to the end of the
+    /// buffer. Given the restrictions on `offset`, this means that
+    /// the size is always greater than zero.
     pub size: Option<wgt::BufferSize>,
 }
 
diff --git a/wgpu-hal/src/metal/device.rs b/wgpu-hal/src/metal/device.rs
index 9ff02a4413..d7c464b730 100644
--- a/wgpu-hal/src/metal/device.rs
+++ b/wgpu-hal/src/metal/device.rs
@@ -16,7 +16,17 @@ struct CompiledShader {
     function: mtl::Function,
     wg_size: mtl::MTLSize,
     wg_memory_sizes: Vec<u32>,
+
+    /// Bindings of WGSL `storage` globals that contain variable-sized arrays.
+    ///
+    /// In order to implement bounds checks and the `arrayLength` function for
+    /// WGSL runtime-sized arrays, we pass the entry point a struct with a
+    /// member for each global variable that contains such an array. That member
+    /// is a `u32` holding the variable's total size in bytes---which is simply
+    /// the size of the `Buffer` supplying that variable's contents for the
+    /// draw call.
     sized_bindings: Vec<naga::ResourceBinding>,
+
     immutable_buffer_mask: usize,
 }
 
@@ -724,6 +734,8 @@ impl crate::Device<super::Api> for super::Device {
                         let end = start + size as usize;
                         bg.buffers
                             .extend(desc.buffers[start..end].iter().map(|source| {
+                                // Given the restrictions on `BufferBinding::offset`,
+                                // this should never be `None`.
                                 let remaining_size =
                                     wgt::BufferSize::new(source.buffer.size - source.offset);
                                 let binding_size = match ty {
diff --git a/wgpu-hal/src/metal/mod.rs b/wgpu-hal/src/metal/mod.rs
index 00a8aa164b..886d145369 100644
--- a/wgpu-hal/src/metal/mod.rs
+++ b/wgpu-hal/src/metal/mod.rs
@@ -584,7 +584,17 @@ struct BufferResource {
     ptr: BufferPtr,
     offset: wgt::BufferAddress,
     dynamic_index: Option<u32>,
+
+    /// The buffer's size, if it is a [`Storage`] binding. Otherwise `None`.
+    ///
+    /// Buffers with the [`wgt::BufferBindingType::Storage`] binding type can
+    /// hold WGSL runtime-sized arrays. When one does, we must pass its size to
+    /// shader entry points to implement bounds checks and WGSL's `arrayLength`
+    /// function. See [`device::CompiledShader::sized_bindings`] for details.
+    ///
+    /// [`Storage`]: wgt::BufferBindingType::Storage
     binding_size: Option<wgt::BufferSize>,
+
     binding_location: u32,
 }
 
@@ -607,7 +617,15 @@ pub struct ShaderModule {
 #[derive(Debug, Default)]
 struct PipelineStageInfo {
     push_constants: Option<PushConstantsInfo>,
+
+    /// The buffer argument table index at which we pass runtime-sized arrays' buffer sizes.
+    ///
+    /// See [`device::CompiledShader::sized_bindings`] for more details.
     sizes_slot: Option<naga::back::msl::Slot>,
+
+    /// Bindings of all WGSL `storage` globals that contain runtime-sized arrays.
+    ///
+    /// See [`device::CompiledShader::sized_bindings`] for more details.
     sized_bindings: Vec<naga::ResourceBinding>,
 }
 
@@ -714,7 +732,28 @@ struct CommandState {
     index: Option<IndexState>,
     raw_wg_size: mtl::MTLSize,
     stage_infos: MultiStageData<PipelineStageInfo>,
+
+    /// Sizes of currently bound [`wgt::BufferBindingType::Storage`] buffers.
+    ///
+    /// Specifically:
+    ///
+    /// - The keys are ['ResourceBinding`] values (that is, the WGSL `@group`
+    ///   and `@binding` attributes) for `var<storage>` global variables in the
+    ///   current module that contain runtime-sized arrays.
+    ///
+    /// - The values are the actual sizes of the buffers currently bound to
+    ///   provide those globals' contents, which are needed to implement bounds
+    ///   checks and the WGSL `arrayLength` function.
+    ///
+    /// For each stage `S` in `stage_infos`, we consult this to find the sizes
+    /// of the buffers listed in [`stage_infos.S.sized_bindings`], which we must
+    /// pass to the entry point.
+    ///
+    /// See [`device::CompiledShader::sized_bindings`] for more details.
+    ///
+    /// [`ResourceBinding`]: naga::ResourceBinding
     storage_buffer_length_map: fxhash::FxHashMap<naga::ResourceBinding, wgt::BufferSize>,
+
     work_group_memory_sizes: Vec<u32>,
     push_constants: Vec<u32>,
 }