Skip to content

Fix optional constraints when used from another generic scope#9149

Closed
juliusikkala wants to merge 3 commits intoshader-slang:masterfrom
juliusikkala:fix-optional-witnesses
Closed

Fix optional constraints when used from another generic scope#9149
juliusikkala wants to merge 3 commits intoshader-slang:masterfrom
juliusikkala:fix-optional-witnesses

Conversation

@juliusikkala
Copy link
Copy Markdown
Contributor

@juliusikkala juliusikkala commented Nov 26, 2025

The issue is that NoneWitness (which is used to "satisfy" optional constraints when the constraint is not fulfilled by T) was getting lowered in-place in the outer generic after the return instruction has been emitted, causing IR looking like this:

[export("_ST4test1Bg1T")]
generic %5      : Type
{
block %6(
                [nameHint("T")]
                param %T1       : type_t):
        [nameHint("B")]
        struct %B       : Type
        {
                field(%a, specialize(%2, %T1, %7))
        }

        return_val(%B)
        witness_table %7        : witness_table_t(Void)(Void);

}

The generated witness table gets inserted after the terminator of block %6, and subsequently causes a variety of havoc inside the compiler, sometimes resulting in an internal error or segmentation fault. This PR ensures that it always gets hoisted into the global scope - NoneWitness can (and IMO should) be shared across all uses.

This is related to the feature with an open proposal in shader-slang/spec#25 and whose implementation was merged in #7422 (not exactly following proper proposal procedure 😬). In any case, the feature has already ended up in the compiler, and has a bug which is fixed by this PR.

@juliusikkala juliusikkala requested a review from a team as a code owner November 26, 2025 19:34
@juliusikkala juliusikkala added the pr: non-breaking PRs without breaking changes label Nov 26, 2025
@csyonghe
Copy link
Copy Markdown
Collaborator

csyonghe commented Dec 2, 2025

looks like the nonewitness does not have a sensible type, which might be fine now but that might be a source of problems in the future.

@csyonghe
Copy link
Copy Markdown
Collaborator

csyonghe commented Dec 2, 2025

I feel like the right fix should be to change the lowering logic, and create the witness table in its own generic, just like how any other ordinary witness tables are created.

@juliusikkala
Copy link
Copy Markdown
Contributor Author

I agree on the types, so I started refactoring that part a bit.

I'm not yet sure how to lower the witness tables as desired here. My understanding is that usually witness tables are generated from the InheritanceDecl:

struct Foo: IFoo
{
    ...
}

For NoneWitnesses, we can't really do that, because they are created when needed and there is no corresponding InheritanceDecl.

@juliusikkala juliusikkala marked this pull request as draft December 3, 2025 21:16
@slangbot
Copy link
Copy Markdown
Contributor

slangbot commented Dec 3, 2025

⚠️ IR Instruction Files Changed

This PR modifies IR instruction definition files. Please review if you need to update the following constants in source/slang/slang-ir.h:

  • k_minSupportedModuleVersion: Should be incremented if you're removing instructions or making breaking changes
  • k_maxSupportedModuleVersion: Should be incremented when adding new instructions

These version numbers help ensure compatibility between different versions of compiled modules.

@csyonghe
Copy link
Copy Markdown
Collaborator

csyonghe commented Dec 4, 2025

You create a fresh IRGenContext, then use that to call emitOuterGeneric passing in the parent generic decl, and null for leafDecl, which will return you a IRGeneric. Then you emit your witness table type and none witness inst using the new context. Once you have the NoneWitness instruction, call finishOuterGenerics on to wrap up the IRGeneric.

@juliusikkala
Copy link
Copy Markdown
Contributor Author

juliusikkala commented Dec 4, 2025

Thanks for the tip, unfortunately I wasn't able to make that work yet (or didn't understand the specifics here).

NoneWitness is a value and not a decl, so emitOuterGenerics() (from DeclLoweringVisitor) is not visible in visitNoneWitness() (ValLoweringVisitor). I tried to sidestep this by creating a temporary DeclLoweringVisitor just to call that function, but ran into other problems with lowerType ("Generic type/value shouldn't be handled here!").

I wonder if it'd be better to somehow add a NoneWitnessDecl in the AST when needed, which would then be referenced by a DeclaredSubtypeWitness.

EDIT: I'm starting to feel like I want to do a larger rewrite to this feature. The initial implementation is too much of a hack, I don't like it at all anymore. The AST-level NoneWitness could probably stay, but I think on the IR side it could be easier to lower optional constraints as Optional<witness_table_t> and just pass none for NoneWitness. It's definitely easier to understand when reading the IR.

@juliusikkala
Copy link
Copy Markdown
Contributor Author

I'll close this now, as I plan to instead refactor the feature and there's not a whole lot I can reuse from here due to the major conflicts with #7968.

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.

3 participants