[wgpu-core] make implicit_pipeline_ids arg optional for users that …

…don't provide IDs
Elabajaba · Jul 17, 2024 · 91924fb · 91924fb
1 parent 7e112ca
commit 91924fb
Show file tree

Hide file tree

Showing 7 changed files with 70 additions and 117 deletions.
diff --git a/deno_webgpu/pipeline.rs b/deno_webgpu/pipeline.rs
@@ -14,8 +14,6 @@ use std::rc::Rc;
 use super::error::WebGpuError;
 use super::error::WebGpuResult;
 
-const MAX_BIND_GROUPS: usize = 8;
-
 pub(crate) struct WebGpuPipelineLayout(
     pub(crate) crate::Instance,
     pub(crate) wgpu_core::id::PipelineLayoutId,
@@ -118,21 +116,12 @@ pub fn op_webgpu_create_compute_pipeline(
         },
         cache: None,
     };
-    let implicit_pipelines = match layout {
-        GPUPipelineLayoutOrGPUAutoLayoutMode::Layout(_) => None,
-        GPUPipelineLayoutOrGPUAutoLayoutMode::Auto(GPUAutoLayoutMode::Auto) => {
-            Some(wgpu_core::device::ImplicitPipelineIds {
-                root_id: None,
-                group_ids: &[None; MAX_BIND_GROUPS],
-            })
-        }
-    };
 
     let (compute_pipeline, maybe_err) = gfx_select!(device => instance.device_create_compute_pipeline(
       device,
       &descriptor,
       None,
-      implicit_pipelines
+      None,
     ));
 
     let rid = state
@@ -397,21 +386,11 @@ pub fn op_webgpu_create_render_pipeline(
         cache: None,
     };
 
-    let implicit_pipelines = match args.layout {
-        GPUPipelineLayoutOrGPUAutoLayoutMode::Layout(_) => None,
-        GPUPipelineLayoutOrGPUAutoLayoutMode::Auto(GPUAutoLayoutMode::Auto) => {
-            Some(wgpu_core::device::ImplicitPipelineIds {
-                root_id: None,
-                group_ids: &[None; MAX_BIND_GROUPS],
-            })
-        }
-    };
-
     let (render_pipeline, maybe_err) = gfx_select!(device => instance.device_create_render_pipeline(
       device,
       &descriptor,
       None,
-      implicit_pipelines
+      None,
     ));
 
     let rid = state

diff --git a/player/src/lib.rs b/player/src/lib.rs
@@ -256,8 +256,8 @@ impl GlobalPlay for wgc::global::Global {
                     implicit_context
                         .as_ref()
                         .map(|ic| wgc::device::ImplicitPipelineIds {
-                            root_id: Some(ic.root_id),
-                            group_ids: wgc::id::as_option_slice(&ic.group_ids),
+                            root_id: ic.root_id,
+                            group_ids: &ic.group_ids,
                         });
                 let (_, error) =
                     self.device_create_compute_pipeline::<A>(device, &desc, Some(id), implicit_ids);
@@ -277,8 +277,8 @@ impl GlobalPlay for wgc::global::Global {
                     implicit_context
                         .as_ref()
                         .map(|ic| wgc::device::ImplicitPipelineIds {
-                            root_id: Some(ic.root_id),
-                            group_ids: wgc::id::as_option_slice(&ic.group_ids),
+                            root_id: ic.root_id,
+                            group_ids: &ic.group_ids,
                         });
                 let (_, error) =
                     self.device_create_render_pipeline::<A>(device, &desc, Some(id), implicit_ids);

diff --git a/wgpu-core/src/device/global.rs b/wgpu-core/src/device/global.rs
@@ -1385,12 +1385,18 @@ impl Global {
 
         let hub = A::hub(self);
 
+        let missing_implicit_pipeline_ids =
+            desc.layout.is_none() && id_in.is_some() && implicit_pipeline_ids.is_none();
+
         let fid = hub.render_pipelines.prepare(id_in);
         let implicit_context = implicit_pipeline_ids.map(|ipi| ipi.prepare(hub));
 
-        let is_auto_layout = desc.layout.is_none();
-
         let error = 'error: {
+            if missing_implicit_pipeline_ids {
+                // TODO: categorize this error as API misuse
+                break 'error pipeline::ImplicitLayoutError::MissingImplicitPipelineIds.into();
+            }
+
             let device = match hub.devices.get(device_id) {
                 Ok(device) => device,
                 Err(_) => break 'error DeviceError::InvalidDeviceId.into(),
@@ -1505,23 +1511,18 @@ impl Global {
                 Err(e) => break 'error e,
             };
 
-            if is_auto_layout {
-                // TODO: categorize the errors below as API misuse
-                let ids = if let Some(ids) = implicit_context.as_ref() {
-                    let group_count = pipeline.layout.bind_group_layouts.len();
-                    if ids.group_ids.len() < group_count {
-                        log::error!(
-                            "Not enough bind group IDs ({}) specified for the implicit layout ({})",
-                            ids.group_ids.len(),
-                            group_count
-                        );
-                        break 'error pipeline::ImplicitLayoutError::MissingIds(group_count as _)
-                            .into();
-                    }
-                    ids
-                } else {
-                    break 'error pipeline::ImplicitLayoutError::MissingIds(0).into();
-                };
+            if let Some(ids) = implicit_context.as_ref() {
+                let group_count = pipeline.layout.bind_group_layouts.len();
+                if ids.group_ids.len() < group_count {
+                    log::error!(
+                        "Not enough bind group IDs ({}) specified for the implicit layout ({})",
+                        ids.group_ids.len(),
+                        group_count
+                    );
+                    // TODO: categorize this error as API misuse
+                    break 'error pipeline::ImplicitLayoutError::MissingIds(group_count as _)
+                        .into();
+                }
 
                 let mut pipeline_layout_guard = hub.pipeline_layouts.write();
                 let mut bgl_guard = hub.bind_group_layouts.write();
@@ -1552,16 +1553,14 @@ impl Global {
 
         let id = fid.assign_error();
 
-        if is_auto_layout {
-            // We also need to assign errors to the implicit pipeline layout and the
-            // implicit bind group layouts.
-            if let Some(ids) = implicit_context {
-                let mut pipeline_layout_guard = hub.pipeline_layouts.write();
-                let mut bgl_guard = hub.bind_group_layouts.write();
-                pipeline_layout_guard.insert_error(ids.root_id);
-                for bgl_id in ids.group_ids {
-                    bgl_guard.insert_error(bgl_id);
-                }
+        // We also need to assign errors to the implicit pipeline layout and the
+        // implicit bind group layouts.
+        if let Some(ids) = implicit_context {
+            let mut pipeline_layout_guard = hub.pipeline_layouts.write();
+            let mut bgl_guard = hub.bind_group_layouts.write();
+            pipeline_layout_guard.insert_error(ids.root_id);
+            for bgl_id in ids.group_ids {
+                bgl_guard.insert_error(bgl_id);
             }
         }
 
@@ -1629,12 +1628,18 @@ impl Global {
 
         let hub = A::hub(self);
 
+        let missing_implicit_pipeline_ids =
+            desc.layout.is_none() && id_in.is_some() && implicit_pipeline_ids.is_none();
+
         let fid = hub.compute_pipelines.prepare(id_in);
         let implicit_context = implicit_pipeline_ids.map(|ipi| ipi.prepare(hub));
 
-        let is_auto_layout = desc.layout.is_none();
-
         let error = 'error: {
+            if missing_implicit_pipeline_ids {
+                // TODO: categorize this error as API misuse
+                break 'error pipeline::ImplicitLayoutError::MissingImplicitPipelineIds.into();
+            }
+
             let device = match hub.devices.get(device_id) {
                 Ok(device) => device,
                 Err(_) => break 'error DeviceError::InvalidDeviceId.into(),
@@ -1703,23 +1708,18 @@ impl Global {
                 Err(e) => break 'error e,
             };
 
-            if is_auto_layout {
-                // TODO: categorize the errors below as API misuse
-                let ids = if let Some(ids) = implicit_context.as_ref() {
-                    let group_count = pipeline.layout.bind_group_layouts.len();
-                    if ids.group_ids.len() < group_count {
-                        log::error!(
-                            "Not enough bind group IDs ({}) specified for the implicit layout ({})",
-                            ids.group_ids.len(),
-                            group_count
-                        );
-                        break 'error pipeline::ImplicitLayoutError::MissingIds(group_count as _)
-                            .into();
-                    }
-                    ids
-                } else {
-                    break 'error pipeline::ImplicitLayoutError::MissingIds(0).into();
-                };
+            if let Some(ids) = implicit_context.as_ref() {
+                let group_count = pipeline.layout.bind_group_layouts.len();
+                if ids.group_ids.len() < group_count {
+                    log::error!(
+                        "Not enough bind group IDs ({}) specified for the implicit layout ({})",
+                        ids.group_ids.len(),
+                        group_count
+                    );
+                    // TODO: categorize this error as API misuse
+                    break 'error pipeline::ImplicitLayoutError::MissingIds(group_count as _)
+                        .into();
+                }
 
                 let mut pipeline_layout_guard = hub.pipeline_layouts.write();
                 let mut bgl_guard = hub.bind_group_layouts.write();
@@ -1750,16 +1750,14 @@ impl Global {
 
         let id = fid.assign_error();
 
-        if is_auto_layout {
-            // We also need to assign errors to the implicit pipeline layout and the
-            // implicit bind group layouts.
-            if let Some(ids) = implicit_context {
-                let mut pipeline_layout_guard = hub.pipeline_layouts.write();
-                let mut bgl_guard = hub.bind_group_layouts.write();
-                pipeline_layout_guard.insert_error(ids.root_id);
-                for bgl_id in ids.group_ids {
-                    bgl_guard.insert_error(bgl_id);
-                }
+        // We also need to assign errors to the implicit pipeline layout and the
+        // implicit bind group layouts.
+        if let Some(ids) = implicit_context {
+            let mut pipeline_layout_guard = hub.pipeline_layouts.write();
+            let mut bgl_guard = hub.bind_group_layouts.write();
+            pipeline_layout_guard.insert_error(ids.root_id);
+            for bgl_id in ids.group_ids {
+                bgl_guard.insert_error(bgl_id);
             }
         }
 

diff --git a/wgpu-core/src/device/mod.rs b/wgpu-core/src/device/mod.rs
@@ -433,18 +433,18 @@ pub struct ImplicitPipelineContext {
 }
 
 pub struct ImplicitPipelineIds<'a> {
-    pub root_id: Option<PipelineLayoutId>,
-    pub group_ids: &'a [Option<BindGroupLayoutId>],
+    pub root_id: PipelineLayoutId,
+    pub group_ids: &'a [BindGroupLayoutId],
 }
 
 impl ImplicitPipelineIds<'_> {
     fn prepare<A: HalApi>(self, hub: &Hub<A>) -> ImplicitPipelineContext {
         ImplicitPipelineContext {
-            root_id: hub.pipeline_layouts.prepare(self.root_id).into_id(),
+            root_id: hub.pipeline_layouts.prepare(Some(self.root_id)).into_id(),
             group_ids: self
                 .group_ids
                 .iter()
-                .map(|id_in| hub.bind_group_layouts.prepare(*id_in).into_id())
+                .map(|id_in| hub.bind_group_layouts.prepare(Some(*id_in)).into_id())
                 .collect(),
         }
     }

diff --git a/wgpu-core/src/id.rs b/wgpu-core/src/id.rs
@@ -77,18 +77,6 @@ impl RawId {
     }
 }
 
-/// Coerce a slice of identifiers into a slice of optional raw identifiers.
-///
-/// There's two reasons why we know this is correct:
-/// * `Option<T>` is guaranteed to be niche-filled to 0's.
-/// * The `T` in `Option<T>` can inhabit any representation except 0's, since
-///   its underlying representation is `NonZero*`.
-pub fn as_option_slice<T: Marker>(ids: &[Id<T>]) -> &[Option<Id<T>>] {
-    // SAFETY: Any Id<T> is repr(transparent) over `Option<RawId>`, since both
-    // are backed by non-zero types.
-    unsafe { std::slice::from_raw_parts(ids.as_ptr().cast(), ids.len()) }
-}
-
 /// An identifier for a wgpu object.
 ///
 /// An `Id<T>` value identifies a value stored in a [`Global`]'s [`Hub`].

diff --git a/wgpu-core/src/pipeline.rs b/wgpu-core/src/pipeline.rs
@@ -186,6 +186,8 @@ pub type ImplicitBindGroupCount = u8;
 #[derive(Clone, Debug, Error)]
 #[non_exhaustive]
 pub enum ImplicitLayoutError {
+    #[error("The implicit_pipeline_ids arg is required")]
+    MissingImplicitPipelineIds,
     #[error("Missing IDs for deriving {0} bind groups")]
     MissingIds(ImplicitBindGroupCount),
     #[error("Unable to reflect the shader {0:?} interface")]

diff --git a/wgpu/src/backend/wgpu_core.rs b/wgpu/src/backend/wgpu_core.rs
@@ -1162,13 +1162,6 @@ impl crate::Context for ContextWgpuCore {
             })
             .collect();
 
-        let implicit_pipeline_ids = match desc.layout {
-            Some(_) => None,
-            None => Some(wgc::device::ImplicitPipelineIds {
-                root_id: None,
-                group_ids: &[None; wgc::MAX_BIND_GROUPS],
-            }),
-        };
         let descriptor = pipe::RenderPipelineDescriptor {
             label: desc.label.map(Borrowed),
             layout: desc.layout.map(|l| l.id.into()),
@@ -1211,7 +1204,7 @@ impl crate::Context for ContextWgpuCore {
             *device,
             &descriptor,
             None,
-            implicit_pipeline_ids
+            None,
         ));
         if let Some(cause) = error {
             if let wgc::pipeline::CreateRenderPipelineError::Internal { stage, ref error } = cause {
@@ -1235,13 +1228,6 @@ impl crate::Context for ContextWgpuCore {
     ) -> (Self::ComputePipelineId, Self::ComputePipelineData) {
         use wgc::pipeline as pipe;
 
-        let implicit_pipeline_ids = match desc.layout {
-            Some(_) => None,
-            None => Some(wgc::device::ImplicitPipelineIds {
-                root_id: None,
-                group_ids: &[None; wgc::MAX_BIND_GROUPS],
-            }),
-        };
         let descriptor = pipe::ComputePipelineDescriptor {
             label: desc.label.map(Borrowed),
             layout: desc.layout.map(|l| l.id.into()),
@@ -1261,7 +1247,7 @@ impl crate::Context for ContextWgpuCore {
             *device,
             &descriptor,
             None,
-            implicit_pipeline_ids
+            None,
         ));
         if let Some(cause) = error {
             if let wgc::pipeline::CreateComputePipelineError::Internal(ref error) = cause {