Issue 14385 in skia: SkSL: WGSL requires that constant-eval expressions do not have overflow/NaN

[johns… via monorail]

Status: Accepted
Owner: johns...@google.com
Area: SkSL
Priority: Medium
Type: Defect

New issue 14385 by johns...@google.com: SkSL: WGSL requires that constant-eval expressions do not have overflow/NaN
https://bugs.chromium.org/p/skia/issues/detail?id=14385

https://www.w3.org/TR/WGSL/#abstract-types

An evaluation of an expression in one of these types must not overflow or produce infinite, NaN, undefined, or indeterminate results.

https://www.w3.org/TR/WGSL/#floating-point-evaluation

It is a shader-creation error if any const-expression of floating-point type evaluates to NaN or infinity.

--

Currently SkSL's strategy for constant-evaluation of overflows/NaNs is to stop constant-evaluation and leave the input code as-is, passing it along to the GPU.

For other backends, this isn't a problem, and leaves the determination of the final result up to the backend in question.

For WGSL, the compiler rejects the code at compilation time. Therefore, we will need a new strategy.

Using `let` forces us out of compile-time evaluation, and can be used to dodge the error. e.g.:

`return inverseSqrt(-1);` --> error
`let x = -1; return inverseSqrt(x);` --> returns mystery value

--
You received this message because:
1. The project was configured to send all issue notifications to this address

You may adjust your notification preferences at:
https://bugs.chromium.org/hosting/settings

[johns… via monorail]

Comment #1 on issue 14385 by johns...@google.com: SkSL: WGSL requires that constant-eval expressions do not have overflow/NaN
https://bugs.chromium.org/p/skia/issues/detail?id=14385#c1

http://review.skia.org/727077

[Git Watcher via monorail]

Comment #2 on issue 14385 by Git Watcher: SkSL: WGSL requires that constant-eval expressions do not have overflow/NaN
https://bugs.chromium.org/p/skia/issues/detail?id=14385#c2

The following revision refers to this bug:
https://skia.googlesource.com/skia/+/18e834916f47a0ebea09d2c79d61fde0554c87f0

commit 18e834916f47a0ebea09d2c79d61fde0554c87f0
Author: John Stiles <johns...@google.com>
Date: Thu Jul 20 16:11:20 2023

Fix Overflow test in WGSL codegen.

The WGSL backend now detects binary expressions of the form
`constant op constant` and replaces the left-hand side with
a let-expression. This trades a WGSL compile-time error for
indeterminate behavior (although the vast majority of backends
will do what we expect and calculate an inf/nan).

Bug: skia:14385
Change-Id: I0392c114504c133b22ea624f61d9df24213888c9
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/726580
Commit-Queue: John Stiles <johns...@google.com>
Auto-Submit: John Stiles <johns...@google.com>
Reviewed-by: Brian Osman <brian...@google.com>

[modify] https://crrev.com/18e834916f47a0ebea09d2c79d61fde0554c87f0/tests/sksl/shared/Overflow.wgsl
[modify] https://crrev.com/18e834916f47a0ebea09d2c79d61fde0554c87f0/src/sksl/codegen/SkSLWGSLCodeGenerator.cpp
[modify] https://crrev.com/18e834916f47a0ebea09d2c79d61fde0554c87f0/tests/sksl/shared/Overflow.skrp
[modify] https://crrev.com/18e834916f47a0ebea09d2c79d61fde0554c87f0/resources/sksl/shared/Overflow.sksl
[modify] https://crrev.com/18e834916f47a0ebea09d2c79d61fde0554c87f0/tests/sksl/shared/Overflow.hlsl
[modify] https://crrev.com/18e834916f47a0ebea09d2c79d61fde0554c87f0/tests/sksl/shared/Overflow.glsl
[modify] https://crrev.com/18e834916f47a0ebea09d2c79d61fde0554c87f0/tests/sksl/shared/Overflow.metal
[modify] https://crrev.com/18e834916f47a0ebea09d2c79d61fde0554c87f0/tests/sksl/shared/Overflow.asm.frag

[Git Watcher via monorail]

Comment #3 on issue 14385 by Git Watcher: SkSL: WGSL requires that constant-eval expressions do not have overflow/NaN
https://bugs.chromium.org/p/skia/issues/detail?id=14385#c3

The following revision refers to this bug:

https://skia.googlesource.com/skia/+/497326dc66cdbf691fc2aae618079f8b706c8bc7

commit 497326dc66cdbf691fc2aae618079f8b706c8bc7
Author: John Stiles <johns...@google.com>
Date: Fri Jul 21 16:40:47 2023

Fix WGSL codegen for intrinsics that might overflow.

This requires different approaches depending on the intrinsic.
For intrinsics with a native WGSL equivalent, we interpret calls
where all arguments are compile-time-constant as an overflow
risk. (Normally, an intrinsic with all compile-time-constant
inputs would have been evaluated at compile time, but we would
have left it as-is if the result overflows.) We can defuse the
WGSL compile-time checks by replacing any argument with a let-
variable; this turns the expression into runtime-evaluated
instead of compile-time evaluated. In practice, we replace the
first argument with a _skTemp value.

For intrinsics where we polyfill the intrinsic, the fix needs
to be handled on a case-by-case basis depending on the contents
of the polyfill. e.g. if the polyfill has `arg0 * arg1`, we
need to make sure that we replace either arg0 or arg1 with a
scratch-let. This is fiddly work, but I've added tests for
each, and fortunately there aren't many cases like this.

Bug: skia:14385
Change-Id: Id882f2bb2ab6e165bea6ce654d24986a85b4c8e6
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/727258
Commit-Queue: Arman Uguray <arma...@google.com>
Auto-Submit: John Stiles <johns...@google.com>
Commit-Queue: John Stiles <johns...@google.com>
Reviewed-by: Arman Uguray <arma...@google.com>

[modify] https://crrev.com/497326dc66cdbf691fc2aae618079f8b706c8bc7/tests/sksl/intrinsics/Refract.hlsl
[modify] https://crrev.com/497326dc66cdbf691fc2aae618079f8b706c8bc7/tests/sksl/intrinsics/MatrixCompMultES2.asm.frag
[modify] https://crrev.com/497326dc66cdbf691fc2aae618079f8b706c8bc7/resources/sksl/intrinsics/Refract.sksl
[modify] https://crrev.com/497326dc66cdbf691fc2aae618079f8b706c8bc7/tests/sksl/intrinsics/MatrixCompMultES2.glsl
[modify] https://crrev.com/497326dc66cdbf691fc2aae618079f8b706c8bc7/resources/sksl/intrinsics/FaceForward.sksl
[modify] https://crrev.com/497326dc66cdbf691fc2aae618079f8b706c8bc7/resources/sksl/intrinsics/Reflect.sksl
[modify] https://crrev.com/497326dc66cdbf691fc2aae618079f8b706c8bc7/infra/bots/tasks.json
[modify] https://crrev.com/497326dc66cdbf691fc2aae618079f8b706c8bc7/resources/sksl/intrinsics/MatrixCompMultES2.sksl
[modify] https://crrev.com/497326dc66cdbf691fc2aae618079f8b706c8bc7/tests/sksl/intrinsics/Refract.metal
[modify] https://crrev.com/497326dc66cdbf691fc2aae618079f8b706c8bc7/tests/sksl/intrinsics/Reflect.metal
[modify] https://crrev.com/497326dc66cdbf691fc2aae618079f8b706c8bc7/tests/sksl/intrinsics/Reflect.hlsl
[modify] https://crrev.com/497326dc66cdbf691fc2aae618079f8b706c8bc7/tests/sksl/intrinsics/Refract.skrp
[modify] https://crrev.com/497326dc66cdbf691fc2aae618079f8b706c8bc7/tests/sksl/intrinsics/Reflect.skrp
[modify] https://crrev.com/497326dc66cdbf691fc2aae618079f8b706c8bc7/tests/sksl/intrinsics/Inverse.wgsl
[modify] https://crrev.com/497326dc66cdbf691fc2aae618079f8b706c8bc7/tests/sksl/intrinsics/FaceForward.asm.frag
[modify] https://crrev.com/497326dc66cdbf691fc2aae618079f8b706c8bc7/tests/sksl/intrinsics/MatrixCompMultES2.skrp
[modify] https://crrev.com/497326dc66cdbf691fc2aae618079f8b706c8bc7/tests/sksl/intrinsics/Reflect.glsl
[modify] https://crrev.com/497326dc66cdbf691fc2aae618079f8b706c8bc7/src/sksl/codegen/SkSLWGSLCodeGenerator.cpp
[modify] https://crrev.com/497326dc66cdbf691fc2aae618079f8b706c8bc7/tests/sksl/intrinsics/FaceForward.hlsl
[modify] https://crrev.com/497326dc66cdbf691fc2aae618079f8b706c8bc7/tests/sksl/intrinsics/Refract.wgsl
[modify] https://crrev.com/497326dc66cdbf691fc2aae618079f8b706c8bc7/src/sksl/codegen/SkSLWGSLCodeGenerator.h
[modify] https://crrev.com/497326dc66cdbf691fc2aae618079f8b706c8bc7/tests/sksl/intrinsics/FaceForward.skrp
[modify] https://crrev.com/497326dc66cdbf691fc2aae618079f8b706c8bc7/tests/sksl/intrinsics/Reflect.asm.frag
[modify] https://crrev.com/497326dc66cdbf691fc2aae618079f8b706c8bc7/tests/sksl/intrinsics/MatrixCompMultES2.wgsl
[modify] https://crrev.com/497326dc66cdbf691fc2aae618079f8b706c8bc7/infra/bots/gen_tasks_logic/dm_flags.go
[modify] https://crrev.com/497326dc66cdbf691fc2aae618079f8b706c8bc7/tests/sksl/intrinsics/Refract.glsl
[modify] https://crrev.com/497326dc66cdbf691fc2aae618079f8b706c8bc7/tests/sksl/intrinsics/MatrixCompMultES2.hlsl
[modify] https://crrev.com/497326dc66cdbf691fc2aae618079f8b706c8bc7/tests/sksl/intrinsics/Inversesqrt.wgsl
[modify] https://crrev.com/497326dc66cdbf691fc2aae618079f8b706c8bc7/tests/sksl/intrinsics/FaceForward.metal
[modify] https://crrev.com/497326dc66cdbf691fc2aae618079f8b706c8bc7/tests/sksl/intrinsics/Reflect.wgsl
[modify] https://crrev.com/497326dc66cdbf691fc2aae618079f8b706c8bc7/tests/sksl/intrinsics/FaceForward.wgsl
[modify] https://crrev.com/497326dc66cdbf691fc2aae618079f8b706c8bc7/tests/sksl/intrinsics/Refract.asm.frag
[modify] https://crrev.com/497326dc66cdbf691fc2aae618079f8b706c8bc7/tests/sksl/intrinsics/Sqrt.wgsl
[modify] https://crrev.com/497326dc66cdbf691fc2aae618079f8b706c8bc7/tests/sksl/intrinsics/MatrixCompMultES2.metal
[modify] https://crrev.com/497326dc66cdbf691fc2aae618079f8b706c8bc7/tests/sksl/intrinsics/FaceForward.glsl

[johns… via monorail]

Updates:
Status: Fixed

Comment #4 on issue 14385 by johns...@google.com: SkSL: WGSL requires that constant-eval expressions do not have overflow/NaN
https://bugs.chromium.org/p/skia/issues/detail?id=14385#c4

(No comment was entered for this change.)