Fix Integer Clamp (#5300)

* Fix Integer Clamp * Changelog
gfx-rs · Feb 26, 2024 · 38419a9 · 38419a9
1 parent d646570
commit 38419a9
Show file tree

Hide file tree

Showing 3 changed files with 58 additions and 6 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -119,6 +119,7 @@ Bottom level categories:
 - Fix timeout when presenting a surface where no work has been done. By @waywardmonkeys in [#5200](https://github.com/gfx-rs/wgpu/pull/5200)
 - Simplify and speed up the allocation of internal IDs. By @nical in [#5229](https://github.com/gfx-rs/wgpu/pull/5229)
 - Fix an issue where command encoders weren't properly freed if an error occurred during command encoding. By @ErichDonGubler in [#5251](https://github.com/gfx-rs/wgpu/pull/5251).
+- Fix behavior of integer `clamp` when `min` argument > `max` argument. By @cwfitzgerald in [#5300](https://github.com/gfx-rs/wgpu/pull/5300).
 - Fix missing validation for `Device::clear_buffer` where `offset + size buffer.size` was not checked when `size` was omitted. By @ErichDonGubler in [#5282](https://github.com/gfx-rs/wgpu/pull/5282).
 
 #### WGL

diff --git a/naga/src/back/glsl/mod.rs b/naga/src/back/glsl/mod.rs
@@ -3138,7 +3138,29 @@ impl<'a, W: Write> Writer<'a, W> {
                     Mf::Abs => "abs",
                     Mf::Min => "min",
                     Mf::Max => "max",
-                    Mf::Clamp => "clamp",
+                    Mf::Clamp => {
+                        let scalar_kind = ctx
+                            .resolve_type(arg, &self.module.types)
+                            .scalar_kind()
+                            .unwrap();
+                        match scalar_kind {
+                            crate::ScalarKind::Float => "clamp",
+                            // Clamp is undefined if min > max. In practice this means it can use a median-of-three
+                            // instruction to determine the value. This is fine according to the WGSL spec for float
+                            // clamp, but integer clamp _must_ use min-max. As such we write out min/max.
+                            _ => {
+                                write!(self.out, "min(max(")?;
+                                self.write_expr(arg, ctx)?;
+                                write!(self.out, ", ")?;
+                                self.write_expr(arg1.unwrap(), ctx)?;
+                                write!(self.out, "), ")?;
+                                self.write_expr(arg2.unwrap(), ctx)?;
+                                write!(self.out, ")")?;
+
+                                return Ok(());
+                            }
+                        }
+                    }
                     Mf::Saturate => {
                         write!(self.out, "clamp(")?;
 

diff --git a/naga/src/back/spv/block.rs b/naga/src/back/spv/block.rs
@@ -731,12 +731,41 @@ impl<'w> BlockContext<'w> {
                         Some(crate::ScalarKind::Uint) => spirv::GLOp::UMax,
                         other => unimplemented!("Unexpected max({:?})", other),
                     }),
-                    Mf::Clamp => MathOp::Ext(match arg_scalar_kind {
-                        Some(crate::ScalarKind::Float) => spirv::GLOp::FClamp,
-                        Some(crate::ScalarKind::Sint) => spirv::GLOp::SClamp,
-                        Some(crate::ScalarKind::Uint) => spirv::GLOp::UClamp,
+                    Mf::Clamp => match arg_scalar_kind {
+                        // Clamp is undefined if min > max. In practice this means it can use a median-of-three
+                        // instruction to determine the value. This is fine according to the WGSL spec for float
+                        // clamp, but integer clamp _must_ use min-max. As such we write out min/max.
+                        Some(crate::ScalarKind::Float) => MathOp::Ext(spirv::GLOp::FClamp),
+                        Some(_) => {
+                            let (min_op, max_op) = match arg_scalar_kind {
+                                Some(crate::ScalarKind::Sint) => {
+                                    (spirv::GLOp::SMin, spirv::GLOp::SMax)
+                                }
+                                Some(crate::ScalarKind::Uint) => {
+                                    (spirv::GLOp::UMin, spirv::GLOp::UMax)
+                                }
+                                _ => unreachable!(),
+                            };
+
+                            let max_id = self.gen_id();
+                            block.body.push(Instruction::ext_inst(
+                                self.writer.gl450_ext_inst_id,
+                                max_op,
+                                result_type_id,
+                                max_id,
+                                &[arg0_id, arg1_id],
+                            ));
+
+                            MathOp::Custom(Instruction::ext_inst(
+                                self.writer.gl450_ext_inst_id,
+                                min_op,
+                                result_type_id,
+                                id,
+                                &[max_id, arg2_id],
+                            ))
+                        }
                         other => unimplemented!("Unexpected max({:?})", other),
-                    }),
+                    },
                     Mf::Saturate => {
                         let (maybe_size, scalar) = match *arg_ty {
                             crate::TypeInner::Vector { size, scalar } => (Some(size), scalar),