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

Semantics when switch init expression and case expression types don't match #4921

Closed
aleino-nv opened this issue Aug 27, 2024 · 6 comments
Closed
Assignees
Labels
goal:quality & productivity Quality issues and issues that impact our productivity coding day to day inside slang

Comments

@aleino-nv
Copy link
Collaborator

What is Slang's semantics when the type of the case expression doesn't match the type in the switch init expression?
Example:

float test(uint x)
{
    switch (x)
    {
        case -1:
            return 1.0f;

        default:
            break;
    }

    return 0.0f;
}

The GLSL specification says:

The type of init-expression in a switch statement must be a scalar integer. The type of the constantexpression value in a case label also 
must be a scalar integer. When any pair of these values is
tested for “equal value” and the types do not match, an implicit conversion will be done to convert
the int to a uint (see “Implicit Conversions”) before the compare is done.

In GLSL this means -1 will be treated like case bitcast<uint>(int(-1)) which means wrap<uint>(int(-1)) in two's complement representation.

Based on GLSL and HLSL output, it seems Slang's de facto semantics is case bitcast<uint>(int(-1)):

  • I see that the GLSL output is just case -1 and as mentioned the GLSL spec reads this as case bitcast<uint>(int(-1))
  • I don't think the HLSL specification mentions this issue, but for the HLSL backend we output case int(-1), and I believe the DXC compiler ends up turning this into case bitcast<uint>(int(-1)).

Can someone confirm?

@aleino-nv
Copy link
Collaborator Author

aleino-nv commented Aug 27, 2024

This came up as part of #4807 because WGSL doesn't allow these kinds of type mismatches, and doesn't try to do any implicit conversions.

https://www.w3.org/TR/WGSL/#switch-statement says:

Type rule precondition: For a single switch statement, the selector expression and all case selector expressions must be of the same concrete integer scalar type.

@bmillsNV bmillsNV assigned bmillsNV and aleino-nv and unassigned bmillsNV Aug 27, 2024
@bmillsNV bmillsNV added this to the Q3 2024 (Summer) milestone Aug 27, 2024
@bmillsNV bmillsNV added the goal:quality & productivity Quality issues and issues that impact our productivity coding day to day inside slang label Aug 27, 2024
@aleino-nv
Copy link
Collaborator Author

This issue was actually a TODO in the code:

void SemanticsStmtVisitor::visitCaseStmt(CaseStmt* stmt)
{
    auto expr = CheckExpr(stmt->expr);

    // coerce to type being switch on, and ensure that value is a compile-time constant
    // The Vals in the AST are pointer-unique, making them easy to check for duplicates
    // by addeing them to a HashSet.
    auto exprVal = tryConstantFoldExpr(expr, ConstantFoldingKind::CompileTime, nullptr);
    auto switchStmt = FindOuterStmt<SwitchStmt>();

    if (!switchStmt)
    {
        getSink()->diagnose(stmt, Diagnostics::caseOutsideSwitch);
    }
    else
    {
        // TODO: need to do some basic matching to ensure the type
        // for the `case` is consistent with the type for the `switch`...
    }

    stmt->expr = expr;
    stmt->exprVal = exprVal;
    stmt->parentStmt = switchStmt;
}

aleino-nv added a commit to aleino-nv/slang that referenced this issue Sep 4, 2024
In WGSL, the switch condition and case types must match.
https://www.w3.org/TR/WGSL/#switch-statement

The Slang WGSL emitter solves this by doing an explicit conversion.
For this reason, pass the switch condition type to the function that emits the cases.

Note that this type mismatch should eventually be fixed in the front-end,
as discussed in issue shader-slang#4921.
However, that is not yet done, so for now this helps to address issue shader-slang#4807.
aleino-nv added a commit to aleino-nv/slang that referenced this issue Sep 5, 2024
Some WGSL compilers complain if e.g. case expression is signed while
condition expression is unsigned.
Insert logic to coerce the case expression to be of the same type as the condition
expression.

This addresses issue shader-slang#4921.
aleino-nv added a commit to aleino-nv/slang that referenced this issue Sep 6, 2024
Some WGSL compilers complain if e.g. case expression is signed while
condition expression is unsigned.
Insert logic to coerce the case expression to be of the same type as the condition
expression.

This addresses issue shader-slang#4921.
aleino-nv added a commit to aleino-nv/slang that referenced this issue Sep 9, 2024
In WGSL, the switch condition and case types must match.
https://www.w3.org/TR/WGSL/#switch-statement

Slang currently allows these types to mismatch, as pointed out in shader-slang#4921.

Issue shader-slang#4921 should eventually be addressed in the front-end by a patch like [1].
However, at the moment that would break Falcor tests.
Thus, this patch temporarily works around the issue in the WGSL emitter only in order to
help resolve shader-slang#4807.

In the future, the Falcor tests should be fixed, this patch should be dropped and [1]
should be merged instead.

[1] a32156e
aleino-nv added a commit to aleino-nv/slang that referenced this issue Sep 9, 2024
In WGSL, the switch condition and case types must match.
https://www.w3.org/TR/WGSL/#switch-statement

Slang currently allows these types to mismatch, as pointed out in shader-slang#4921.

Issue shader-slang#4921 should eventually be addressed in the front-end by a patch like [1].
However, at the moment that would break Falcor tests.
Thus, this patch temporarily works around the issue in the WGSL emitter only in order to
help resolve shader-slang#4807.

In the future, the Falcor tests should be fixed, this patch should be dropped and [1]
should be merged instead.

[1] a32156e
csyonghe pushed a commit that referenced this issue Sep 9, 2024
* Add WGSL as a target

This is required for #4807.

* C-like emitter: Allow the function header emission to be overloaded

WGSL-style function headers are pretty different from normal C-style headers:

Normal C-style headers:
 ReturnType Func(...)
 void VoidFunc(...)

WGSL-style headers:
  fn Func(...) -> ReturnType
  fn VoidFunc(...)

This change allows the header style to be overloaded, in order to accomodate WGSL-style
headers as required to resolve issue #4807, but retains normal C-style headers as the
default implementation.

[1] https://www.w3.org/TR/WGSL/#function-declaration-sec

* C-like emitter: Allow emission of switch case selectors to be overloaded

The C-like emitter will emit code like this:

    switch(a.x)
    {

    case 0:
    case 1:
    {
       ...
    } break;

    ...

    }

This is not allowed in WGSL. Instead, selectors for cases that share a body must [1] be
separated by commas, like this:

    switch(a.x)
    {

    case 0, 1:
    {
       ...
    } break;

    ...

    }

To prepare for addressing issue #4807, this patch makes the emission of switch case
selectors overloadable.

[1] https://www.w3.org/TR/WGSL/#syntax-case_selectors

* C-like emitter: Support WGSL-style declarations

This patch helps to address issue 4807.

C-like languages declare variables like this:
i32 a;

WGSL declares variables like this:
var a : i32

The patch introduces overloads so that the forthcoming WGSL emitter can output WGSL-style
declarations, which helps to resolve #4807.

* C-like emitter: Support overloading of declarators

Unlike C-like languages, WGSL does not support the following types at the syntax level,
via declarators:
- arrays
- pointers
- references

For this reason, this patch introduces support for overloading the declarator emitter,
in order to help address issue #4807.

C-like languages:
int a[3]; // Array-ness of type is mixed into the "declarator"

WGSL:
var a : array<int, 3>; // Array-ness of type is part of the... type_specifier!

* C-like emitter: Allow struct declaration separator to be overridden

C-like languages use ';' as a separator, and languages like e.g. WGSL use ','.
This change prepares for addressing issue #4807.

* C-like emitter: Allow overriding of whether pointer-like syntax is necessary

Things like e.g. structured buffers map to "ptr-to-array" in WGSL, but ptr-typed
expressions don't always need C-style pointer-like syntax.
Therefore, make it overrideable whether or not such syntax is emitted in various cases in
order to address #4807.

* C-like emitter: Emit parenthesis to avoid warning about & and + precedence

This helps with #4807 because WGSL compilers (e.g. Tint) treat absence of parenthesis as
an error.

* C-like emitter: Add hook for emitting struct field attributes

WGSL requires @align attributes to specify explicit field alignment in certain cases.
Thus, this patch prepares for addressing #4807.

* C-like emitter: Add hook for emitting global param types

Declarations of structured buffers map to global array declarations in WGSL.
However, in all other cases such as when structured buffers are used in operands, their
types map to *ptr*-to-array.
This patch makes it possible for the WGSL back-end to say that structured buffers
generally map to "ptr-to-array" types, but still have a special case of just "array" when
declaring the global shader parameter.

Thus, this patch helps with addressing #4807.

* IR lowering: Use std140 for WGSL uniform buffers

This patch just cuts out some logic that prevented std140 to be chosen for WGSL uniform
buffers.

Note that WGSL buffers in the uniform address space is not quite std140, but for now it's
close enough to avoid compile issues.

Later on, a custom layout should be created for WGSL uniform buffers.
When that's done, this change will be revisited, but for now it helps to resolve #4807.

* Don't emit line directives in WGSL by default

WGSL does not support line directives [1].
The plan currently seems to be to instead support source-map [2].
This is part of addressing issue #4807.

[1] gpuweb/gpuweb#606
[2] https://github.com/mozilla/source-map

* WGSL IR legalization: Map SV's

The implementation closely follows the cooresponding one for Metal.

Supported:
- DispatchThreadID
- GroupID
- GroupThreadID
- GroupThreadID

Unsupported:
- GSInstanceID

This is not complete, but it helps to address #4807.

* WGSL emitter: Add support for basic language constructs

A lot of the basics are added in order to generate correct WGSL code for basic Slang language constructs.
This addresses issue #4807.

This adds support for at least the following:
- statments
 - if statements
 - ternary operator
 - while statement
 - for statements
 - variable declarations
 - switch statements
  - Note: Slang may emit non-constant case expressions, see issue 4834
- literals
 - integer literals
  - u?int[16|32|64]_t
  - float and half literals
  - bool literals
  - vector literals and splatting (e.g 1.xxx)
- function definitions
- assignments
 - +=, *=, /=
 - array assignments
 - vector assignments/updates
  - swizzles of other vectors
  - from matrix rows ('m[i]' notation)
  - from matrix cols (using swizzle notation, e.g 'm._11_12_13')
 - matrix assignments/updates
  - to rows ('m[i]' notation)
  - to cols (using swizzle notation, e.g 'm._11_12_13')
- declarations
 - arrays

[1] https://www.w3.org/TR/WGSL/#syntax-switch_body

* Add some WGSL capabilities

This patch registers some WGSL capabilities required to pass many of the initial compute
shader compile tests.
Many capabilities still remain to be added -- this is just an initial set to help resolve
issue #4807.

- asint
- min and max
- cos and sin
- all and any

* WGSL and C-like emitters: Add hack to bitcast case expression

In WGSL, the switch condition and case types must match.
https://www.w3.org/TR/WGSL/#switch-statement

Slang currently allows these types to mismatch, as pointed out in #4921.

Issue #4921 should eventually be addressed in the front-end by a patch like [1].
However, at the moment that would break Falcor tests.
Thus, this patch temporarily works around the issue in the WGSL emitter only in order to
help resolve #4807.

In the future, the Falcor tests should be fixed, this patch should be dropped and [1]
should be merged instead.

[1] a32156e
@aleino-nv
Copy link
Collaborator Author

As we found out when merging the initial WGSL commit the front-end fix worked, but will first require a fix in Falcor.

@aleino-nv
Copy link
Collaborator Author

Blocking Falcor issue to do the fix
NVIDIAGameWorks/Falcor#449

@csyonghe
Copy link
Collaborator

Let's fork Falcor to shader-slang/ and use it for CI testing.

Then we can fix the error in our fork of Falcor to unblock this change.

@csyonghe
Copy link
Collaborator

csyonghe commented Nov 5, 2024

This is done.

@csyonghe csyonghe closed this as completed Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
goal:quality & productivity Quality issues and issues that impact our productivity coding day to day inside slang
Projects
None yet
Development

No branches or pull requests

3 participants