Make Statement::Atomic::result optional.

Rather than introducing an entirely new statement variant
`AtomicNoResult`, make `Statement::Atomic::result` an `Option`. This
reduces the patch size by about a third, and has no effect on snapshot
output other than the direct IR dumps.

In HLSL output, don't bother to generated "discard" temporaries for
functions where the final `original_value` parameter is not required.
This improves snapshot output.

Improve documentation for `Expression::AtomicResult`,
`Statement::Atomic`, and the new `valid::Capabilities` flags.

Rewrite `Atomic` statement validation. Consolidate validation of
`AtomicResult` expressions with this; gfx-rs#5771 ensures that there is
indeed some `Atomic` statement referring to every `AtomicResult`
expression, so we can be sure it will be validated.

Fixes gfx-rs#5742.

Author: jimblandy

naga/src/back/dot/mod.rs

@@ -244,23 +244,16 @@ impl StatementGraph {
                     value,
                     result,
                 } => {
-                    self.emits.push((id, result));
+                    if let Some(result) = result {
+                        self.emits.push((id, result));
+                    }
                     self.dependencies.push((id, pointer, "pointer"));
                     self.dependencies.push((id, value, "value"));
                     if let crate::AtomicFunction::Exchange { compare: Some(cmp) } = *fun {
                         self.dependencies.push((id, cmp, "cmp"));
                     }
                     "Atomic"
                 }
-                S::AtomicNoReturn {
-                    pointer,
-                    fun: _,
-                    value,
-                } => {
-                    self.dependencies.push((id, pointer, "pointer"));
-                    self.dependencies.push((id, value, "value"));
-                    "AtomicNoReturn"
-                }
                 S::WorkGroupUniformLoad { pointer, result } => {
                     self.emits.push((id, result));
                     self.dependencies.push((id, pointer, "pointer"));

naga/src/back/glsl/mod.rs

@@ -96,15 +96,6 @@ impl crate::AtomicFunction {
     }
 }
 
-impl crate::AtomicFunctionNoReturn {
-    const fn to_glsl(self) -> &'static str {
-        match self {
-            Self::Min => "Min",
-            Self::Max => "Max",
-        }
-    }
-}
-
 impl crate::AddressSpace {
     const fn is_buffer(&self) -> bool {
         match *self {
@@ -2377,11 +2368,13 @@ impl<'a, W: Write> Writer<'a, W> {
                 result,
             } => {
                 write!(self.out, "{level}")?;
-                let res_name = format!("{}{}", back::BAKE_PREFIX, result.index());
-                let res_ty = ctx.resolve_type(result, &self.module.types);
-                self.write_value_type(res_ty)?;
-                write!(self.out, " {res_name} = ")?;
-                self.named_expressions.insert(result, res_name);
+                if let Some(result) = result {
+                    let res_name = format!("{}{}", back::BAKE_PREFIX, result.index());
+                    let res_ty = ctx.resolve_type(result, &self.module.types);
+                    self.write_value_type(res_ty)?;
+                    write!(self.out, " {res_name} = ")?;
+                    self.named_expressions.insert(result, res_name);
+                }
 
                 let fun_str = fun.to_glsl();
                 write!(self.out, "atomic{fun_str}(")?;
@@ -2403,20 +2396,6 @@ impl<'a, W: Write> Writer<'a, W> {
                 self.write_expr(value, ctx)?;
                 writeln!(self.out, ");")?;
             }
-            Statement::AtomicNoReturn {
-                pointer,
-                ref fun,
-                value,
-            } => {
-                write!(self.out, "{level}")?;
-
-                let fun_str = fun.to_glsl();
-                write!(self.out, "atomic{fun_str}(")?;
-                self.write_expr(pointer, ctx)?;
-                write!(self.out, ", ")?;
-                self.write_expr(value, ctx)?;
-                writeln!(self.out, ");")?;
-            }
             Statement::RayQuery { .. } => unreachable!(),
             Statement::SubgroupBallot { result, predicate } => {
                 write!(self.out, "{level}")?;

naga/src/back/hlsl/conv.rs

@@ -233,13 +233,3 @@ impl crate::AtomicFunction {
         }
     }
 }
-
-impl crate::AtomicFunctionNoReturn {
-    /// Return the HLSL suffix for the `InterlockedXxx` method.
-    pub(super) const fn to_hlsl_suffix(self) -> &'static str {
-        match self {
-            Self::Min => "Min",
-            Self::Max => "Max",
-        }
-    }
-}

naga/src/back/hlsl/writer.rs

@@ -1919,11 +1919,20 @@ impl<'a, W: fmt::Write> super::Writer<'a, W> {
                 result,
             } => {
                 write!(self.out, "{level}")?;
-                let res_name = format!("{}{}", back::BAKE_PREFIX, result.index());
-                match func_ctx.info[result].ty {
-                    proc::TypeResolution::Handle(handle) => self.write_type(module, handle)?,
-                    proc::TypeResolution::Value(ref value) => {
-                        self.write_value_type(module, value)?
+                let res_name = match result {
+                    None => None,
+                    Some(result) => {
+                        let name = format!("{}{}", back::BAKE_PREFIX, result.index());
+                        match func_ctx.info[result].ty {
+                            proc::TypeResolution::Handle(handle) => {
+                                self.write_type(module, handle)?
+                            }
+                            proc::TypeResolution::Value(ref value) => {
+                                self.write_value_type(module, value)?
+                            }
+                        };
+                        write!(self.out, " {name}; ")?;
+                        Some((result, name))
                     }
                 };
 
@@ -1934,7 +1943,6 @@ impl<'a, W: fmt::Write> super::Writer<'a, W> {
                     .unwrap();
 
                 let fun_str = fun.to_hlsl_suffix();
-                write!(self.out, " {res_name}; ")?;
                 match pointer_space {
                     crate::AddressSpace::WorkGroup => {
                         write!(self.out, "Interlocked{fun_str}(")?;
@@ -1970,56 +1978,16 @@ impl<'a, W: fmt::Write> super::Writer<'a, W> {
                     _ => {}
                 }
                 self.write_expr(module, value, func_ctx)?;
-                writeln!(self.out, ", {res_name});")?;
-                self.named_expressions.insert(result, res_name);
-            }
-            Statement::AtomicNoReturn {
-                pointer,
-                ref fun,
-                value,
-            } => {
-                write!(self.out, "{level}")?;
-                let res_name = format!("{}_discard{}", back::BAKE_PREFIX, pointer.index());
-                match func_ctx.info[value].ty {
-                    proc::TypeResolution::Handle(handle) => self.write_type(module, handle)?,
-                    proc::TypeResolution::Value(ref value) => {
-                        self.write_value_type(module, value)?
-                    }
-                };
-
-                // Validation ensures that `pointer` has a `Pointer` type.
-                let pointer_space = func_ctx
-                    .resolve_type(pointer, &module.types)
-                    .pointer_space()
-                    .unwrap();
 
-                let fun_str = fun.to_hlsl_suffix();
-                write!(self.out, " {res_name}; ")?;
-                match pointer_space {
-                    crate::AddressSpace::WorkGroup => {
-                        write!(self.out, "Interlocked{fun_str}(")?;
-                        self.write_expr(module, pointer, func_ctx)?;
-                    }
-                    crate::AddressSpace::Storage { .. } => {
-                        let var_handle = self.fill_access_chain(module, pointer, func_ctx)?;
-                        // The call to `self.write_storage_address` wants
-                        // mutable access to all of `self`, so temporarily take
-                        // ownership of our reusable access chain buffer.
-                        let chain = mem::take(&mut self.temp_access_chain);
-                        let var_name = &self.names[&NameKey::GlobalVariable(var_handle)];
-                        write!(self.out, "{var_name}.Interlocked{fun_str}(")?;
-                        self.write_storage_address(module, &chain, func_ctx)?;
-                        self.temp_access_chain = chain;
-                    }
-                    ref other => {
-                        return Err(Error::Custom(format!(
-                            "invalid address space {other:?} for atomic statement"
-                        )))
-                    }
+                // The `original_value` out parameter is optional for all the
+                // `Interlocked` functions we generate other than
+                // `InterlockedExchange`.
+                if let Some((result, name)) = res_name {
+                    write!(self.out, ", {name}")?;
+                    self.named_expressions.insert(result, name);
                 }
-                write!(self.out, ", ")?;
-                self.write_expr(module, value, func_ctx)?;
-                writeln!(self.out, ", {res_name});")?;
+
+                writeln!(self.out, ");")?;
             }
             Statement::WorkGroupUniformLoad { pointer, result } => {
                 self.write_barrier(crate::Barrier::WORK_GROUP, level)?;

naga/src/back/msl/mod.rs

@@ -1,6 +1,9 @@
 /*!
 Backend for [MSL][msl] (Metal Shading Language).
 
+This backend does not support the [`SHADER_INT64_ATOMIC_ALL_OPS`][all-atom]
+capability.
+
 ## Binding model
 
 Metal's bindings are flat per resource. Since there isn't an obvious mapping
@@ -24,6 +27,8 @@ For the result type, if it's a structure, we re-compose it with a temporary valu
 holding the result.
 
 [msl]: https://developer.apple.com/metal/Metal-Shading-Language-Specification.pdf
+[all-atom]: crate::valid::Capabilities::SHADER_INT64_ATOMIC_ALL_OPS
+
 */
 
 use crate::{arena::Handle, proc::index, valid::ModuleInfo};
@@ -661,30 +666,3 @@ fn test_error_size() {
     use std::mem::size_of;
     assert_eq!(size_of::<Error>(), 32);
 }
-
-impl crate::AtomicFunction {
-    fn to_msl(self) -> Result<&'static str, Error> {
-        Ok(match self {
-            Self::Add => "fetch_add",
-            Self::Subtract => "fetch_sub",
-            Self::And => "fetch_and",
-            Self::InclusiveOr => "fetch_or",
-            Self::ExclusiveOr => "fetch_xor",
-            Self::Min => "fetch_min",
-            Self::Max => "fetch_max",
-            Self::Exchange { compare: None } => "exchange",
-            Self::Exchange { compare: Some(_) } => Err(Error::FeatureNotImplemented(
-                "atomic CompareExchange".to_string(),
-            ))?,
-        })
-    }
-}
-
-impl crate::AtomicFunctionNoReturn {
-    const fn to_msl(self) -> &'static str {
-        match self {
-            Self::Min => "min",
-            Self::Max => "max",
-        }
-    }
-}

naga/src/back/msl/writer.rs

@@ -3058,27 +3058,21 @@ impl<W: Write> Writer<W> {
                     value,
                     result,
                 } => {
+                    // This backend supports `SHADER_INT64_ATOMIC_MIN_MAX` but not
+                    // `SHADER_INT64_ATOMIC_ALL_OPS`, so we can assume that if `result` is
+                    // `Some`, we are not operating on a 64-bit value, and that if we are
+                    // operating on a 64-bit value, `result` is `None`.
                     write!(self.out, "{level}")?;
-                    let res_name = format!("{}{}", back::BAKE_PREFIX, result.index());
-                    self.start_baking_expression(result, &context.expression, &res_name)?;
-                    self.named_expressions.insert(result, res_name);
-                    let fun_str = fun.to_msl()?;
-                    self.put_atomic_operation(pointer, fun_str, value, &context.expression)?;
-                    // done
-                    writeln!(self.out, ";")?;
-                }
-                crate::Statement::AtomicNoReturn {
-                    pointer,
-                    ref fun,
-                    value,
-                } => {
-                    write!(self.out, "{level}")?;
-                    let fun_str =
-                        if context.expression.resolve_type(value).scalar_width() != Some(8) {
-                            fun.with_return().to_msl()?
-                        } else {
-                            fun.to_msl()
-                        };
+                    let fun_str = if let Some(result) = result {
+                        let res_name = format!("{}{}", back::BAKE_PREFIX, result.index());
+                        self.start_baking_expression(result, &context.expression, &res_name)?;
+                        self.named_expressions.insert(result, res_name);
+                        fun.to_msl()?
+                    } else if context.expression.resolve_type(value).scalar_width() == Some(8) {
+                        fun.to_msl_64_bit()?
+                    } else {
+                        fun.to_msl()?
+                    };
 
                     self.put_atomic_operation(pointer, fun_str, value, &context.expression)?;
                     // done
@@ -5931,3 +5925,31 @@ fn test_stack_size() {
         }
     }
 }
+
+impl crate::AtomicFunction {
+    fn to_msl(self) -> Result<&'static str, Error> {
+        Ok(match self {
+            Self::Add => "fetch_add",
+            Self::Subtract => "fetch_sub",
+            Self::And => "fetch_and",
+            Self::InclusiveOr => "fetch_or",
+            Self::ExclusiveOr => "fetch_xor",
+            Self::Min => "fetch_min",
+            Self::Max => "fetch_max",
+            Self::Exchange { compare: None } => "exchange",
+            Self::Exchange { compare: Some(_) } => Err(Error::FeatureNotImplemented(
+                "atomic CompareExchange".to_string(),
+            ))?,
+        })
+    }
+
+    fn to_msl_64_bit(self) -> Result<&'static str, Error> {
+        Ok(match self {
+            Self::Min => "min",
+            Self::Max => "max",
+            _ => Err(Error::FeatureNotImplemented(
+                "64-bit atomic operation other than min/max".to_string(),
+            ))?,
+        })
+    }
+}

naga/src/back/pipeline_constants.rs

@@ -617,7 +617,9 @@ fn adjust_stmt(new_pos: &[Handle<Expression>], stmt: &mut Statement) {
         } => {
             adjust(pointer);
             adjust(value);
-            adjust(result);
+            if let Some(ref mut result) = *result {
+                adjust(result);
+            }
             match *fun {
                 crate::AtomicFunction::Exchange {
                     compare: Some(ref mut compare),
@@ -634,14 +636,6 @@ fn adjust_stmt(new_pos: &[Handle<Expression>], stmt: &mut Statement) {
                 | crate::AtomicFunction::Exchange { compare: None } => {}
             }
         }
-        crate::Statement::AtomicNoReturn {
-            ref mut pointer,
-            ref mut value,
-            fun: _,
-        } => {
-            adjust(pointer);
-            adjust(value);
-        }
         Statement::WorkGroupUniformLoad {
             ref mut pointer,
             ref mut result,