Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use and() and or() functions for logical-AND and OR #6310

Conversation

jkwak-work
Copy link
Collaborator

With this commit, Slang will emit function calls to and() and or() for the logical-AND and logical-OR when the operands are non-scalar and the target profile is SM6.0 and above. This is required change from SM6.0.

For WGSL, there is no operator overloadings of && and || when the operands are non-scalar. Unlike HLSL, WGSL also don't have and() nor or(). Alternatively, we can use select().

Closes #6307

@jkwak-work jkwak-work added the pr: non-breaking PRs without breaking changes label Feb 7, 2025
@jkwak-work jkwak-work self-assigned this Feb 7, 2025
@jkwak-work jkwak-work requested a review from a team as a code owner February 7, 2025 00:27
@jkwak-work jkwak-work marked this pull request as draft February 7, 2025 00:46
With this commit, Slang will emit function calls to `and()` and `or()`
for the logical-AND and logical-OR when the operands are non-scalar and
the target profile is SM6.0 and above. This is required change from
SM6.0.

For WGSL, there is no operator overloadings of `&&` and `||` when the
operands are non-scalar. Unlike HLSL, WGSL also don't have `and()` nor
`or()`. Alternatively, we can use `select()`.
@jkwak-work jkwak-work force-pushed the feature/logical_operator_short_circuiting_for_dxc_1_8_and_above branch from 4324aaf to 2612c2a Compare February 7, 2025 00:57
@jkwak-work jkwak-work marked this pull request as ready for review February 7, 2025 02:24
@jkwak-work
Copy link
Collaborator Author

It looks like my change is failing on a few Falcor image tests.
I will take a look.
internal/renderpasses/test_MegakernelPathTracerGBuffer_d3d12

@jkwak-work
Copy link
Collaborator Author

The failure on Falcor is due to the outdated DX validator.
The latest validator doesn't have any problem with it.

With my change, the following code causes the validation to fail:

float3 outColor_1 = outColor_0 + select(vector<bool,3>(and((int3)gData_0.params_0.clampSamples_0, int3((path_2.L_0) > (gData_0.params_0.clampThreshold_0)))), (float3)gData_0.params_0.clampThreshold_0, path_2.L_0);

Before my change, it was:

float3 outColor_1 = outColor_0 + (vector<bool,3>(((int3)gData_0.params_0.clampSamples_0) && int3((path_2.L_0) > (gData_0.params_0.clampThreshold_0))) ? (float3)gData_0.params_0.clampThreshold_0 : path_2.L_0);

Note that the following passes the validation, so the problem is on the type given to and() is not bool type.

float3 outColor_1 = outColor_0 + select(vector<bool,3>(and((bool3)(int3)gData_0.params_0.clampSamples_0, (bool3)int3((path_2.L_0) > (gData_0.params_0.clampThreshold_0)))), (float3)gData_0.params_0.clampThreshold_0, path_2.L_0);

@csyonghe
Copy link
Collaborator

csyonghe commented Feb 7, 2025

I think we want to make sure we convert the operands of and to bool when emitting HLSL.

@jkwak-work
Copy link
Collaborator Author

I am working on this to handle the type properly.

When I changed the test case little bit to cover the case, I started to see SPIRV validation failure too.
So I think it is worth handling it at IR level properly.

We have legalizeBinaryOp function, so I will put my implementation similar to it.

csyonghe
csyonghe previously approved these changes Feb 7, 2025
@jkwak-work
Copy link
Collaborator Author

I am fixing the Metal case.

@jkwak-work
Copy link
Collaborator Author

Yeah¡ CI all green

@jkwak-work jkwak-work merged commit 57b09a8 into shader-slang:master Feb 8, 2025
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: non-breaking PRs without breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Logical operator short-circuit errors after upgrading to dxc 1.8 and above
2 participants