[tint][ast] Fix RemovePhonies transform with short-circuited fn call
The RHS of a constant short-circuited expression should not be emitted as a call statement as this may introduce validation failures that were previously ignored as the RHS is not evaluated.
Bug: 339704114
Change-Id: Ic2d1fc25248f63077289cba0041a13480aadde67
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/187691
Commit-Queue: Ben Clayton <bclayton@google.com>
Auto-Submit: Ben Clayton <bclayton@google.com>
Reviewed-by: James Price <jrprice@google.com>
Commit-Queue: James Price <jrprice@google.com>
diff --git a/src/tint/lang/wgsl/ast/transform/remove_phonies.cc b/src/tint/lang/wgsl/ast/transform/remove_phonies.cc
index ff33316..2994e09 100644
--- a/src/tint/lang/wgsl/ast/transform/remove_phonies.cc
+++ b/src/tint/lang/wgsl/ast/transform/remove_phonies.cc
@@ -32,6 +32,7 @@
#include <utility>
#include <vector>
+#include "src/tint/lang/core/evaluation_stage.h"
#include "src/tint/lang/wgsl/ast/traverse_expressions.h"
#include "src/tint/lang/wgsl/program/clone_context.h"
#include "src/tint/lang/wgsl/program/program_builder.h"
@@ -85,7 +86,8 @@
return TraverseAction::Skip;
}
if (call->Target()->IsAnyOf<sem::Function, sem::BuiltinFn>() &&
- call->HasSideEffects()) {
+ call->HasSideEffects() &&
+ call->Stage() != core::EvaluationStage::kNotEvaluated) {
side_effects.push_back(expr);
return TraverseAction::Skip;
}
diff --git a/src/tint/lang/wgsl/ast/transform/remove_phonies.h b/src/tint/lang/wgsl/ast/transform/remove_phonies.h
index ecb6d6e..665dc7f 100644
--- a/src/tint/lang/wgsl/ast/transform/remove_phonies.h
+++ b/src/tint/lang/wgsl/ast/transform/remove_phonies.h
@@ -38,6 +38,8 @@
/// RemovePhonies is a Transform that removes all phony-assignment statements,
/// while preserving function call expressions in the RHS of the assignment that
/// may have side-effects. It also removes calls to builtins that return a constant value.
+/// @note RemovePhonies must be run after the PromoteSideEffectsToDecl transform, otherwise `f` in
+/// `_ = cond && f()` may get hoisted to a call statement without the short-circuiting conditional.
class RemovePhonies final : public Castable<RemovePhonies, Transform> {
public:
/// Constructor
diff --git a/src/tint/lang/wgsl/ast/transform/remove_phonies_test.cc b/src/tint/lang/wgsl/ast/transform/remove_phonies_test.cc
index af6c01d..dc1dbcd 100644
--- a/src/tint/lang/wgsl/ast/transform/remove_phonies_test.cc
+++ b/src/tint/lang/wgsl/ast/transform/remove_phonies_test.cc
@@ -496,5 +496,30 @@
EXPECT_EQ(expect, str(got));
}
+TEST_F(RemovePhoniesTest, ConstShortCircuit) {
+ auto* src = R"(
+fn a(v : i32) -> i32 {
+ return v;
+}
+
+fn b() {
+ _ = false && (a(4294967295) < a(a(4294967295)));
+}
+)";
+
+ auto* expect = R"(
+fn a(v : i32) -> i32 {
+ return v;
+}
+
+fn b() {
+}
+)";
+
+ auto got = Run<RemovePhonies>(src);
+
+ EXPECT_EQ(expect, str(got));
+}
+
} // namespace
} // namespace tint::ast::transform
diff --git a/test/tint/bug/chromium/339704114.wgsl b/test/tint/bug/chromium/339704114.wgsl
new file mode 100644
index 0000000..933e912
--- /dev/null
+++ b/test/tint/bug/chromium/339704114.wgsl
@@ -0,0 +1,6 @@
+fn a(v : i32) -> i32 { return v; }
+
+@compute @workgroup_size(1)
+fn b() {
+ _ = false && (0 < a(4294967295));
+}
diff --git a/test/tint/bug/chromium/339704114.wgsl.expected.dxc.hlsl b/test/tint/bug/chromium/339704114.wgsl.expected.dxc.hlsl
new file mode 100644
index 0000000..4c2bc20
--- /dev/null
+++ b/test/tint/bug/chromium/339704114.wgsl.expected.dxc.hlsl
@@ -0,0 +1,8 @@
+int a(int v) {
+ return v;
+}
+
+[numthreads(1, 1, 1)]
+void b() {
+ return;
+}
diff --git a/test/tint/bug/chromium/339704114.wgsl.expected.fxc.hlsl b/test/tint/bug/chromium/339704114.wgsl.expected.fxc.hlsl
new file mode 100644
index 0000000..4c2bc20
--- /dev/null
+++ b/test/tint/bug/chromium/339704114.wgsl.expected.fxc.hlsl
@@ -0,0 +1,8 @@
+int a(int v) {
+ return v;
+}
+
+[numthreads(1, 1, 1)]
+void b() {
+ return;
+}
diff --git a/test/tint/bug/chromium/339704114.wgsl.expected.glsl b/test/tint/bug/chromium/339704114.wgsl.expected.glsl
new file mode 100644
index 0000000..e6c3a0f
--- /dev/null
+++ b/test/tint/bug/chromium/339704114.wgsl.expected.glsl
@@ -0,0 +1,10 @@
+#version 310 es
+
+void b() {
+}
+
+layout(local_size_x = 1, local_size_y = 1, local_size_z = 1) in;
+void main() {
+ b();
+ return;
+}
diff --git a/test/tint/bug/chromium/339704114.wgsl.expected.msl b/test/tint/bug/chromium/339704114.wgsl.expected.msl
new file mode 100644
index 0000000..a45ab87
--- /dev/null
+++ b/test/tint/bug/chromium/339704114.wgsl.expected.msl
@@ -0,0 +1,11 @@
+#include <metal_stdlib>
+
+using namespace metal;
+int a(int v) {
+ return v;
+}
+
+kernel void b() {
+ return;
+}
+
diff --git a/test/tint/bug/chromium/339704114.wgsl.expected.spvasm b/test/tint/bug/chromium/339704114.wgsl.expected.spvasm
new file mode 100644
index 0000000..c0ed813
--- /dev/null
+++ b/test/tint/bug/chromium/339704114.wgsl.expected.spvasm
@@ -0,0 +1,25 @@
+; SPIR-V
+; Version: 1.3
+; Generator: Google Tint Compiler; 0
+; Bound: 10
+; Schema: 0
+ OpCapability Shader
+ OpMemoryModel Logical GLSL450
+ OpEntryPoint GLCompute %b "b"
+ OpExecutionMode %b LocalSize 1 1 1
+ OpName %a "a"
+ OpName %v "v"
+ OpName %b "b"
+ %int = OpTypeInt 32 1
+ %1 = OpTypeFunction %int %int
+ %void = OpTypeVoid
+ %6 = OpTypeFunction %void
+ %a = OpFunction %int None %1
+ %v = OpFunctionParameter %int
+ %5 = OpLabel
+ OpReturnValue %v
+ OpFunctionEnd
+ %b = OpFunction %void None %6
+ %9 = OpLabel
+ OpReturn
+ OpFunctionEnd
diff --git a/test/tint/bug/chromium/339704114.wgsl.expected.wgsl b/test/tint/bug/chromium/339704114.wgsl.expected.wgsl
new file mode 100644
index 0000000..693ed7f
--- /dev/null
+++ b/test/tint/bug/chromium/339704114.wgsl.expected.wgsl
@@ -0,0 +1,8 @@
+fn a(v : i32) -> i32 {
+ return v;
+}
+
+@compute @workgroup_size(1)
+fn b() {
+ _ = (false && (0 < a(4294967295)));
+}
