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

Fix lowering logic of nested generics. #4091

Merged
merged 4 commits into from
May 2, 2024

Conversation

csyonghe
Copy link
Collaborator

@csyonghe csyonghe commented May 2, 2024

Fixes #4048.

The issue exposed in #4048 is that our generics implementation does not support nested generics that introduce type constraints depending on another type parameter of the outer generic.

Specifically, this code wasn't allowed:

struct Outer<T>
{
    // `U` has constraint that is dependent on `T`
     void funcInner<U:IArray<T>>(U arr);
}

There isn't any fundamental reason that prevented us to handle this correctly. The main issue found is our IR lowering logic isn't correctly generating code that represent the type of a nesting generic inst. Fixing the bugs in the lowering logic allows everything to run correctly.

Note that a related issue is that in the future we we ever want to allow a where clause to specify depending generic params.
For example:

void func<T,U>(U arr) where U:IArray<T>
{}

This means that this function will lower into the following IR:

%g_func = IRGeneric
{
     IRBlock
     {
          %T = IRParam : Type;
          %U = IRParam : Type;
          %w_U = IRParam : %wt;
          %s = specialize(%IArray, %T);
          %wt = IRWitnessTableType(%s);
          %func = IRFunc {...}
          return %func;
     }
}

Note that %w_U has type %wt which is defined after w_U because in our IR all params must appear before any other insts.
However this means that we are violating another rule that says all insts must dominate their uses, where %wt is not dominating %w_U.

So in this PR we also try to relax our IR rule to allow all IRParams to use things that are defined later in the same block to allow this generic to be legally represented. But this means that our IR cloning logic need to change accordingly to work with the relaxed rule.

Copy link
Collaborator

@saipraveenb25 saipraveenb25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool! This significantly expands on the set of types we can express.

The logic seems similar in spirit to IROutOfOrderCloneContext. We may want to unify them if more uses for out of order cloning crop up.

Does this not cause an issue with IR validation?

@csyonghe
Copy link
Collaborator Author

csyonghe commented May 2, 2024

@saipraveenb25 It seems that we are only validating operands and not types of an inst. In this case it is the type of an IRParam that is defined later in the block, and it is never checked by validation logic.

@csyonghe
Copy link
Collaborator Author

csyonghe commented May 2, 2024

Just did more debugging and some of my claims are false. I am going to update my description here.

@csyonghe csyonghe changed the title Support generic constraints that are dependent on another generic param. Fix lowering logic of nested generics. May 2, 2024
@csyonghe csyonghe merged commit 1863fe1 into shader-slang:master May 2, 2024
15 of 16 checks passed
djohansson pushed a commit to djohansson/slang that referenced this pull request Sep 21, 2024
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

Successfully merging this pull request may close these issues.

Exception at address when defining member function in generic class with another generic parameter
2 participants