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

Add API method to specialize function reference with argument types #4966

Merged

Conversation

saipraveenb25
Copy link
Collaborator

@saipraveenb25 saipraveenb25 commented Aug 29, 2024

  • Adds FunctionReflection::specializeWithArgTypes()
  • Fixes an issue with findFunctionByName returning a reference to a generic instead of a function-decl.

Fixes: #4887

@saipraveenb25 saipraveenb25 added goal:client exploration Speculative work for a potential use case. pr: new feature pr: non-breaking PRs without breaking changes and removed goal:client exploration Speculative work for a potential use case. labels Aug 29, 2024
@@ -1510,7 +1510,7 @@ namespace Slang
outGenericArgs.add(argExpr);
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

trailing space.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed!

@@ -1348,6 +1347,59 @@ SLANG_NO_THROW slang::TypeReflection* SLANG_MCALL Linkage::specializeType(
return asExternal(specializedType);
}

DeclRef<GenericDecl> getGenericParentDeclRef(
Copy link
Collaborator

Choose a reason for hiding this comment

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

can just use getInnermostGenericParent?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a bit different unfortunately.
This function returns the decl-ref of the generic-decl while getInnermostGenericParent returns the decl-ref of the inner decl of the closest generic-decl (this is so that substitutions are available)

List<Type*> argTypes,
DiagnosticSink* sink)
{
SharedSemanticsContext sharedSemanticsContext(this, nullptr, sink);
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should really start to hold a semantic visitor on the linkage.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The linkage now holds a ref-ptr to a shared semantics context that is used for all the reflection functions.

Copy link
Collaborator

@csyonghe csyonghe left a comment

Choose a reason for hiding this comment

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

The fact that we represent generic decl as a parent of the actual func/type decl is an implementation detail and subject to future changes. We really don't want to expose this detail via a publicly visible API that would lock us in this representation.

I would like us to figure out a way to hide this detail from the user, so that the user can always just see the actual decl itself without having to deal with GenericDecl explicitly.

@@ -2589,6 +2589,7 @@ extern "C"
SLANG_API SlangReflectionType* spReflectionFunction_GetResultType(SlangReflectionFunction* func);
SLANG_API SlangReflectionGeneric* spReflectionFunction_GetGenericContainer(SlangReflectionFunction* func);
SLANG_API SlangReflectionFunction* spReflectionFunction_applySpecializations(SlangReflectionFunction* func, SlangReflectionGeneric* generic);
SLANG_API SlangReflectionFunction* spReflectionFunction_specializeWithArgTypes(SlangReflectionFunction* func, SlangInt argTypeCount, SlangReflectionType* const* argTypes);
Copy link
Collaborator

Choose a reason for hiding this comment

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

What does apply specializations do? Is it just get default specialized DeclRef? This imo is an internal detail in generic representation and should not be exposed in the api. What are the scenarios that this is needed from the user?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The idea here is that the generic parameters & their arguments (if any) are represented through a SlangReflectionGeneric. The fact that the generic is represented as a parent in the hierarchy is not directly exposed to the user. It's just that every SlangReflectionFunction/SlangReflectionType/SlangReflectionVariable declaration may have an associated generic which can be obtained using getGenericContainer. Further, each declaration can be nested inside another one, so you can traverse all generic parameters affecting a reflected function/variable/type by using getOuterGenericContainer repeatedly.

In our case, we're actually not referencing the parent generic decl directly, the decl-ref for SlangReflectionFunction and SlangReflectionGeneric are actually the same decl-ref, but they expose different methods (the generic reflection is exclusively for looking through generic parameters, and is common to functions, variables & types)

Each SlangReflectionGeneric effectively represents a set of specializations. applySpecializations creates a specialized decl-ref out of an unspecialized one by applying the bag of specializations to the referenced decl-ref.
In this case, I can lookup an unspecialized function, extract the generic-reflection, set some parameters on the generic-reflection, then apply it to the function-reflection to get the specialized version.

I couldn't come up with a sleeker way to do this, given that we can have many levels of generics, all of which may or may not already have specializations. This idea of putting specializations behind a special generic-reflection is to just avoid having to duplicate the generic-specific methods for all the different types of reflections.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see. As long as we make sure the API doesn't assume generic decl is the parent of the decl itself, and just treat generic as a different aspect of the same decl, then we should be fine.

Copy link
Collaborator

Choose a reason for hiding this comment

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

we should be careful on the ast - children enumeration logic to not list the generic decl as the children, but instead the inner decl.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, this is currently the case. If the decl-walking API hits a generic decl, the user must cast it to SlangReflectionGeneric and call getInnerDecl() to get the inner node.

@saipraveenb25 saipraveenb25 merged commit d866c0b into shader-slang:master Sep 16, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: new feature pr: non-breaking PRs without breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reflection API: Allow specialization of functions and types from a list of argument types
2 participants