Support SV_DispatchGrid semantic in a nested record#6931
Support SV_DispatchGrid semantic in a nested record#6931damyanp merged 6 commits intomicrosoft:mainfrom
Conversation
llvm-beanz
left a comment
llvm-beanz
left a comment
There was a problem hiding this comment.
We should also make sure we have error detection in Sema for the case of multiple fields in a single or nested structure definition having SV_DispatchGrid applied. Since the CodeGen can only handle a single declaration, we should error if a structure has more than one annotated field.
| // Collect any non-virtual bases. | ||
| SmallVector<const CXXRecordDecl *, 4> Bases; | ||
| for (const CXXBaseSpecifier &Base : RD->bases()) { | ||
| if (!Base.isVirtual() && !Base.getType()->isDependentType()) |
There was a problem hiding this comment.
I assume it isn't actually possible in HLSL to have virtual base classes right? The language certainly shouldn't allow them, so I think this check is probably unnecessary.
There was a problem hiding this comment.
No, virtual base classes aren't possible - but the dependent type is, so we do need that check here. This condition just follows the style and layout of similar checks elsewhere - the isVirtual() check could safely be removed.
There was a problem hiding this comment.
Yea, I think we should remove the isVirtual check since that will always be false and I can't imagine any world where we'd be adding virtual inheritance to HLSL...
|
The presence of more than one SV_DispatchGrid field is already detected in Sema, and covers the cases of base classes and nested records. |
| [NumThreads(32,1,1)] | ||
| void node5(DispatchNodeInputRecord<Record5> input) {} | ||
| // CHECK: , i32 1, ![[SVDG_5:[0-9]+]] | ||
| // CHECK: [[SVDG_5]] = !{i32 20, i32 5, i32 2} |
There was a problem hiding this comment.
What about some test cases with templates?
Something like:
template <typename T>
struct Base {
T DG : SV_DispatchGrid;
};
struct Derived1 : Base<uint3> {
int4 x;
};
[Shader("node")]
[NodeLaunch("broadcasting")]
[NodeMaxDispatchGrid(32,16,1)]
[NumThreads(32,1,1)]
void node6(DispatchNodeInputRecord<Derived1 > input) {}
template <typename T>
struct Derived2 : Base<T> {
T Y;
};
[Shader("node")]
[NodeLaunch("broadcasting")]
[NodeMaxDispatchGrid(32,16,1)]
[NumThreads(32,1,1)]
void node7(DispatchNodeInputRecord<Derived2<uint2> > input) {}
template <typename T>
struct Derived3 {
Derived2<T> V;
};
[Shader("node")]
[NodeLaunch("broadcasting")]
[NodeMaxDispatchGrid(32,16,1)]
[NumThreads(32,1,1)]
void node8(DispatchNodeInputRecord< Derived3 <uint3> > input) {}There was a problem hiding this comment.
Great idea! I've updated the test to include these cases.
|
@llvm-beanz - could you have a look at this with some urgency please (we want it in the upcoming release) when you're back? |
pow2clk
left a comment
pow2clk
left a comment
There was a problem hiding this comment.
Looks good! I think the parts I'm most concerned about are the repeated checks of parent classes and that I might not understand the motive for sorting base offsets. The other stuff is minor.
pow2clk
left a comment
pow2clk
left a comment
There was a problem hiding this comment.
I don't mind the unneeded virtual check, but perhaps @llvm-beanz feels differently.
| if (FindDispatchGridSemantic(Base, SDGRec, BaseOffset)) | ||
| DXASSERT(!Base.getType()->isDependentType(), | ||
| "Node Record with dependent base class not caught by Sema"); | ||
| if (Base.isVirtual() || Base.getType()->isDependentType()) |
There was a problem hiding this comment.
It's minor since it should never fire, but I think the agreement per #6931 (comment) was to remove the virtual check.
There was a problem hiding this comment.
I had removed this unnecessary check, but it looks as if it was accidentally reinstated with a subsequent commit. I have removed it again.
The SV_DispatchGrid DXIL metadata for a node input record was not generated in cases where: - the field with the SV_DispatchGrid semantic was in a nested record - the field with the SV_DispatchGrid semantic was in a record field - the field with the SV_DispatchGrid semantic was inherited from a base record - in any combinations of the above Added FindDispatchGridSemantic() to be used by the AddHLSLNodeRecordTypeInfo() function, and added a test case. Fixes microsoft#6928
Remove the check for a virtual base class from the code in FindDispatchGridSemantic() as virtual classes can't appear in HLSL code.
Added test cases to cover nested SV_DispatchGrid used in records using templates.
Adopting code suggested in review - there can only be one concrete base so the sort step isn't required. Co-authored-by: Tex Riddell <[email protected]>
The unnecessary check for a virtual base class had previously been removed but was apparently accidentally reintroduced by a subsequent commit.
tex3d
left a comment
tex3d
left a comment
There was a problem hiding this comment.
Looks good. Only one minor nit on new variable names not following coding standard.
Trivial change to amend local variable names to follow the coding standard.
damyanp
left a comment
damyanp
left a comment
There was a problem hiding this comment.
I see two approvals that have been made stale by a change to some variable names. So I'm going to reapprove and merge this.
5 participants
The SV_DispatchGrid DXIL metadata for a node input record was not generated in cases where:
Added FindDispatchGridSemantic() to be used by the AddHLSLNodeRecordTypeInfo() function, and added a test case.
Fixes #6928