shader-slang / slang

Making it easier to work with shaders
http://shader-slang.com
MIT License
2.16k stars 186 forks source link

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

Closed aleino-nv closed 2 days ago

aleino-nv commented 2 months ago

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)):

Can someone confirm?

aleino-nv commented 2 months ago

This came up as part of https://github.com/shader-slang/slang/issues/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.

aleino-nv commented 2 months ago

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 commented 1 month ago

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 commented 1 month ago

Blocking Falcor issue to do the fix https://github.com/NVIDIAGameWorks/Falcor/issues/449

csyonghe commented 1 month ago

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 commented 2 days ago

This is done.