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

Check whether array element is fully specialized #6000

Merged
merged 5 commits into from
Jan 8, 2025

Conversation

kaizhangNV
Copy link
Contributor

close #5776

When we start specialize a "specialize" IR, we should
make sure all the elements are fully specialized, but
we miss checking the elements of an array. This change
will check the it.

close shader-slang#5776

When we start specialize a "specialize" IR, we should
make sure all the elements are fully specialized, but
we miss checking the elements of an array. This change
will check the it.
@kaizhangNV kaizhangNV requested a review from a team as a code owner January 3, 2025 21:46
@kaizhangNV kaizhangNV added the pr: non-breaking PRs without breaking changes label Jan 3, 2025
@@ -123,6 +123,12 @@ struct SpecializationContext
}
return false;
}
case kIROp_ArrayType:
{
auto array = as<IRArrayType>(inst);
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder why this case is necessary.
If we have an type that is T.Differential[N], then it shall be encoded as IRArrayType(IRLookupWitness(w, "Differential"), N), and since IRLookupWitness has operand w, it won't appear in global scope, and therefore the IRArrayType itself won't appear directly in the global scope, and therefore it won't be reported as true by the default case logic.

Basically, if the element type is not fully specialized, then the array type itself shouldn't appear in the global scope. What IR are you seeing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this case this necessary, the unit test can reproduce the case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I ask this question because in theory this shouldn’t happen, otherwise we will have the same issue for Ptr type and all other kinds of wrapper types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you dump the IR from the unit test, you will find out:

Ptr(specialize(%84, specialize(%150, Float, 1 : Int, %168)))	= get_field_addr(%callx5Fdata, %result)

this could result as:

	struct %GradInBuffer	: Type
	{
		field(%gradx5Fin, specialize(%82, lookupWitness( witness_table_t(%IDifferentiable)(Array(Float, 1)),   %37)))
	}

and finally this lookup table will give us

Array(lookupWitness(%168, %37), %N6)

here is the case I need to handle.

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. In this case we do need to also handle all other opcodes by recursively checking their operands. Because we could have PtrType(lookupWitness()), or ArrayType(int, lookupwitness()), etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I always hit this:

type argument 'float[1]' does not conform to the required interface 'IFoo'

not sure what's wrong regarding extending Array to conform to IFoo.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In your syntax you specified T:IFoo, since float !: IFoo, Array<float,1> !: IFoo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I see the problem, I forgot removing that line.

OK, I will see if I can craft a shader to reproduce this issue.

Copy link
Collaborator

@csyonghe csyonghe Jan 6, 2025

Choose a reason for hiding this comment

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

For a repro, you probably need to define an associatedtype in the interface for lookupwitness to occur inside an array type

for example, something like:

interface IFoo
{
      associatedtype FA : IFoo;
     int get();
}
struct X : IFoo { typealias FA = X;  int get() { return -1; } }
extension<T, int n> Array<T, n> : IFoo
{
      typealias FA = X;
}

int test<T:IFoo>(T.FA v[2]) {
     return v[0].get();
}

...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wrote another shader a little bit different from yours, but now I can get stable reproduce.

@kaizhangNV
Copy link
Contributor Author

Update the patch to cover all wrapper types, also added more test cases.

}
case kIROp_TextureType:
case kIROp_VectorType:
case kIROp_MatrixType:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also: TupleType, OptionalType, TypePack.

we should define a isBuiltinGenericType() utility in slang-it-util that returns true for these opcodes so that we don’t have to duplicate these switch cases all over the place.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure.

An extra question, in the test, I actually want to cover the case like:

StructuedBuffer< StructuredBuffer > input;

but I'm struggled with how to feed data to this type.

From the generated code, I do see we handle this situation correctly, but I didn't cover this in the test as I don't know how to feed data.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is an invalid type. Slang should check and report error on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

miss the inner generic parameter, is this the invalid type?
StructuedBuffer< StructuredBuffer<float> >

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also: TupleType, OptionalType, TypePack.

we should define a isBuiltinGenericType() utility in slang-it-util that returns true for these opcodes so that we don’t have to duplicate these switch cases all over the place.

Done of adding this.

@csyonghe csyonghe merged commit 1a56f58 into shader-slang:master Jan 8, 2025
16 checks passed
@kaizhangNV kaizhangNV deleted the spec_array branch January 8, 2025 21:50
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.

Invalid HLSL generated for bwd_diff function
2 participants