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

[HLSL 2021] Ternary error with non scalar #4070

Closed
arnaud-neny opened this issue Nov 8, 2021 · 4 comments
Closed

[HLSL 2021] Ternary error with non scalar #4070

arnaud-neny opened this issue Nov 8, 2021 · 4 comments

Comments

@arnaud-neny
Copy link

While compiling this code, we got this error using the hlsl 2021: condition for short-circuiting ternary operator must be scalar (for the n.xy >= 0.0 ? t : -t)

float2 PackNormalOct(float3 n) {
    n *= rcp(dot(abs(n), 1.0));
    float t = saturate(-n.z);
    return n.xy + (n.xy >= 0.0 ? t : -t);
}

float3 UnpackNormalOct(float2 f) {
    float3 n = float3(f.x, f.y, 1.0 - abs(f.x) - abs(f.y));
    float t = max(-n.z, 0.0);
    n.xy += n.xy >= 0.0 ? -t : t;
    return normalize(n);
}
@arnaud-neny arnaud-neny changed the title Ternary error with non scalar [HLSL 2021] Ternary error with non scalar Nov 8, 2021
@pow2clk
Copy link
Member

pow2clk commented Nov 15, 2021

Hi @Mrarnal!

This is expected behavior described in this wiki page: https://github.com/microsoft/DirectXShaderCompiler/wiki/HLSL-2021#logical-operation-short-circuiting-for-scalars . The intent is to open up logical short-circuiting for scalar operators not possible with vectors.

You can get the pre-2021 behavior using the new select() intrinsic. In this case, the code above would have the form

float2 PackNormalOct(float3 n) {
    n *= rcp(dot(abs(n), 1.0));
    float t = saturate(-n.z);
    return n.xy + select(n.xy >= 0.0, t, -t);
}

float3 UnpackNormalOct(float2 f) {
    float3 n = float3(f.x, f.y, 1.0 - abs(f.x) - abs(f.y));
    float t = max(-n.z, 0.0);
    n.xy += select(n.xy >= 0.0, -t , t);
    return normalize(n);
}

@pow2clk pow2clk closed this as completed Nov 15, 2021
@arnaud-neny
Copy link
Author

arnaud-neny commented Nov 16, 2021

Hi @pow2clk
Ah ok, thank you for the info. But why not keeping the default behavior and change while compiling to select instead of outputting an error? It will help a lot of projects to avoid refactor and different codepaths depending on cross-platorms shaders.

@pow2clk
Copy link
Member

pow2clk commented Nov 16, 2021

The information we gathered revealed that, most of the time when vectors were used in logical binary and ternary operators, it was with the expectation of short-circuit behavior. Instead, the result was slow code that executed every possibility.

We wanted to enable the short-circuit behavior that authors have come to expect from C++. A big part of the HLSL 2021 plan was to narrow the gap between HLSL and C++. We didn't want the same operators to have such a dramatic difference in behavior based only on the arity of the operands.

Also, irritating as it is, we wanted shader authors who were using these operators with vectors expecting short circuit behavior to take notice of the places they were and possibly, instead of converting them to use select() or the other new intrinsics, they might want to convert them to scalars possibly using any() or all() as appropriate to get the short-circuit benefits.

As a result of this issue and others, we will be adding more information to the error message to make it clearer what the intent is in the future.

@arnaud-neny
Copy link
Author

Great, make sense! Thank you for your time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants