-
Notifications
You must be signed in to change notification settings - Fork 827
Implement support for groupshared arg attribute #8013
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
base: main
Are you sure you want to change the base?
Changes from all commits
e71c32a
8b20e9e
c3437fa
fdca3fc
c3cadac
8511e4e
03e93a7
c20b77a
3519631
210be47
85b23c0
a6f890e
c9f1ab1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4945,6 +4945,12 @@ static void ProcessDeclAttribute(Sema &S, Scope *scope, Decl *D, | |
| handleSimpleAttribute<NoDuplicateAttr>(S, D, Attr); | ||
| break; | ||
| case AttributeList::AT_NoInline: | ||
| if (S.LangOpts.HLSL) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not quite sure I understand this change. Would we otherwise not parse the attributes of the parameter without this?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the basic handling wouldn't check if a parameter of a function with the noinline attribute has the groupshared attribute. |
||
| bool Handled = false; | ||
| hlsl::HandleDeclAttributeForHLSL(S, D, Attr, Handled); | ||
| if (Handled) | ||
| break; | ||
| } | ||
| handleSimpleAttribute<NoInlineAttr>(S, D, Attr); | ||
| break; | ||
| case AttributeList::AT_NoInstrumentFunction: // Interacts with -pg. | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -14474,6 +14474,21 @@ void Sema::DiagnoseHLSLDeclAttr(const Decl *D, const Attr *A) { | |||||||
| HLSLExternalSource *ExtSource = HLSLExternalSource::FromSema(this); | ||||||||
| const bool IsGCAttr = isa<HLSLGloballyCoherentAttr>(A); | ||||||||
| const bool IsRCAttr = isa<HLSLReorderCoherentAttr>(A); | ||||||||
| const bool IsExportAttr = isa<HLSLExportAttr>(A); | ||||||||
| const bool IsNoInlineAttr = isa<NoInlineAttr>(A); | ||||||||
| if (IsExportAttr || IsNoInlineAttr) { | ||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can remove an indentation level by just retrning early if neither condition is true
Suggested change
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The function doesn't return if this is false.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the if statement should probably not be returning either.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
🤦♂️ |
||||||||
| if (const FunctionDecl *FD = dyn_cast<FunctionDecl>(D)) { | ||||||||
| for (ParmVarDecl *PVD : FD->parameters()) { | ||||||||
| if (PVD->hasAttr<HLSLGroupSharedAttr>()) { | ||||||||
| Diag(A->getLocation(), diag::err_hlsl_varmodifiersna) | ||||||||
| << "groupshared" | ||||||||
| << "export/noinline" | ||||||||
| << "parameter"; | ||||||||
| return; | ||||||||
| } | ||||||||
| } | ||||||||
| } | ||||||||
| } | ||||||||
| if (IsGCAttr || IsRCAttr) { | ||||||||
| const ValueDecl *TD = cast<ValueDecl>(D); | ||||||||
| if (TD->getType()->isDependentType()) | ||||||||
|
|
@@ -14655,6 +14670,8 @@ void hlsl::HandleDeclAttributeForHLSL(Sema &S, Decl *D, const AttributeList &A, | |||||||
| VD->setType( | ||||||||
| S.Context.getAddrSpaceQualType(VD->getType(), DXIL::kTGSMAddrSpace)); | ||||||||
| } | ||||||||
| if (ParmVarDecl *VD = dyn_cast<ParmVarDecl>(D)) | ||||||||
| VD->setType(S.Context.getLValueReferenceType(VD->getType())); | ||||||||
| break; | ||||||||
| case AttributeList::AT_HLSLUniform: | ||||||||
| declAttr = ::new (S.Context) HLSLUniformAttr( | ||||||||
|
|
@@ -14996,6 +15013,7 @@ void hlsl::HandleDeclAttributeForHLSL(Sema &S, Decl *D, const AttributeList &A, | |||||||
| } | ||||||||
|
|
||||||||
| if (declAttr != nullptr) { | ||||||||
| S.DiagnoseHLSLDeclAttr(D, declAttr); | ||||||||
| DXASSERT_NOMSG(Handled); | ||||||||
| D->addAttr(declAttr); | ||||||||
|
|
||||||||
|
|
@@ -15749,7 +15767,17 @@ bool Sema::DiagnoseHLSLDecl(Declarator &D, DeclContext *DC, Expr *BitWidth, | |||||||
| break; | ||||||||
| case AttributeList::AT_HLSLGroupShared: | ||||||||
| isGroupShared = true; | ||||||||
| if (!isGlobal) { | ||||||||
| if (isParameter && getLangOpts().HLSLVersion < hlsl::LangStd::v202x) | ||||||||
| Diag(pAttr->getLoc(), diag::warn_hlsl_groupshared_202x); | ||||||||
| if (isParameter && (usageIn || usageOut)) { | ||||||||
| Diag(pAttr->getLoc(), diag::err_hlsl_varmodifiersna) | ||||||||
| << (usageIn && usageOut ? "'inout'" | ||||||||
| : usageIn ? "'in'" | ||||||||
| : "'out'") | ||||||||
| << pAttr->getName() << declarationType; | ||||||||
| result = false; | ||||||||
| } | ||||||||
| if (!(isGlobal || isParameter)) { | ||||||||
| Diag(pAttr->getLoc(), diag::err_hlsl_varmodifierna) | ||||||||
| << pAttr->getName() << declarationType << pAttr->getRange(); | ||||||||
| result = false; | ||||||||
|
|
@@ -15786,6 +15814,10 @@ bool Sema::DiagnoseHLSLDecl(Declarator &D, DeclContext *DC, Expr *BitWidth, | |||||||
| Diag(pAttr->getLoc(), diag::err_hlsl_usage_not_on_parameter) | ||||||||
| << pAttr->getName() << pAttr->getRange(); | ||||||||
| result = false; | ||||||||
| } else if (isGroupShared) { | ||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it guaranteed that Otherwise, we might need to handle the opposite case above
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at the cases for inout and for groupshared I think it is handled regardless of order, but good point and I think this points to a test is missing for this. |
||||||||
| Diag(pAttr->getLoc(), diag::err_hlsl_varmodifiersna) | ||||||||
| << pAttr->getName() << "'groupshared'" << declarationType; | ||||||||
| result = false; | ||||||||
| } | ||||||||
| if (!IsUsageAttributeCompatible(pAttr->getKind(), usageIn, usageOut)) { | ||||||||
| Diag(pAttr->getLoc(), diag::err_hlsl_duplicate_parameter_usages) | ||||||||
|
|
||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| // RUN: %dxc -E main -T cs_6_0 -HV 202x -fcgl %s | FileCheck %s | ||
|
|
||
| groupshared float4 SharedArr[64]; | ||
|
|
||
| // CHECK-LABEL: define internal void @"\01?fn@@YAXAGAY0EA@$$CAV?$vector@M$03@@M@Z"([64 x <4 x float>] addrspace(3)* dereferenceable(1024) %Arr, float %F) | ||
| // CHECK: [[ArrIdx:%.*]] = getelementptr inbounds [64 x <4 x float>], [64 x <4 x float>] addrspace(3)* %Arr, i32 0, i32 5 | ||
| // CHECK-NEXT: store <4 x float> {{.*}}, <4 x float> addrspace(3)* [[ArrIdx]], align 4 | ||
| void fn(groupshared float4 Arr[64], float F) { | ||
| float4 tmp = F.xxxx; | ||
| Arr[5] = tmp; | ||
| } | ||
|
|
||
| [numthreads(4,1,1)] | ||
| void main() { | ||
| fn(SharedArr, 6.0); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| // RUN: %dxc -E main -T cs_6_3 -HV 202x -fcgl %s | FileCheck %s | ||
|
|
||
| groupshared float4x4 SharedData; | ||
|
|
||
| // CHECK-LABEL: @"\01?fn1@@YAXAGAV?$matrix@M$03$03@@@Z" | ||
| // CHECK: [[M1:%.*]] = addrspacecast %class.matrix.float.4.4 addrspace(3)* %Sh to %class.matrix.float.4.4* | ||
| // CHECK: [[A:%.*]] = call <4 x float>* @"dx.hl.subscript.colMajor[].rn.<4 x float>* (i32, %class.matrix.float.4.4*, i32, i32, i32, i32)"(i32 1, %class.matrix.float.4.4* [[M1]], i32 0, i32 4, i32 8, i32 12) | ||
| // CHECK: [[B:%.*]] = getelementptr <4 x float>, <4 x float>* [[A]], i32 0, i32 1 | ||
| // CHECK: store float 5.000000e+00, float* [[B]] | ||
| // CHECK: ret void | ||
| void fn1(groupshared float4x4 Sh) { | ||
| Sh[0][1] = 5.0; | ||
| } | ||
|
|
||
| [numthreads(4,1,1)] | ||
| void main(uint3 TID : SV_GroupThreadID) { | ||
| fn1(SharedData); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| // RUN: %dxc -E main -T cs_6_0 -HV 202x -fcgl %s | FileCheck %s | ||
|
|
||
| // Verify we are calling the correct overloads | ||
| void fn(groupshared float4 Arr[2]); | ||
| void fn(inout float4 Arr[2]); | ||
|
|
||
| void fn2(groupshared int4 Shared); | ||
| void fn2(int4 Local); | ||
|
|
||
| // CHECK-LABEL: define void @main() | ||
| [numthreads(4,1,1)] | ||
| void main() { | ||
| float4 Local[2] = {1.0.xxxx, 2.0.xxxx}; | ||
| // CHECK: call void @"\01?fn@@YAXY01$$CAV?$vector@M$03@@@Z"([2 x <4 x float>]* %Local) | ||
| fn(Local); | ||
|
|
||
| // CHECK: call void @"\01?fn2@@YAXV?$vector@H$03@@@Z"(<4 x i32> | ||
| fn2(11.xxxx); | ||
| } | ||
|
|
||
| void fn(groupshared float4 Arr[2]) { | ||
hekota marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| Arr[1] = 7.0.xxxx; | ||
| } | ||
|
|
||
| // CHECK-LABEL: define internal void @"\01?fn@@YAXY01$$CAV?$vector@M$03@@@Z"([2 x <4 x float>]* noalias %Arr) | ||
| void fn(inout float4 Arr[2]) { | ||
| Arr[1] = 5.0.xxxx; | ||
| } | ||
|
|
||
| void fn2(groupshared int4 Shared) { | ||
| Shared.x = 10; | ||
| } | ||
|
|
||
| // CHECK-LABEL: define internal void @"\01?fn2@@YAXV?$vector@H$03@@@Z"(<4 x i32> %Local) | ||
| void fn2(int4 Local) { | ||
| int X = Local.y; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| // RUN: %dxc -E main -T cs_6_2 -enable-16bit-types -HV 202x -fcgl %s | FileCheck %s | ||
|
|
||
| groupshared uint16_t SharedData; | ||
|
|
||
| // Make sure the mangling changes for groupshared parameter annotation is reflected in the function signature (the first G) | ||
| // The mangled function signature for void fn(uint16_t Sh) should be @"\01?fn1@@YAXAAG@Z" (without the extra G) | ||
| // CHECK-LABEL: @"\01?fn1@@YAXAGAG@Z" | ||
| // CHECK: store i16 5, i16 addrspace(3)* %Sh, align 4 | ||
| void fn1(groupshared uint16_t Sh) { | ||
| Sh = 5; | ||
| } | ||
|
|
||
| [numthreads(4, 1, 1)] | ||
| void main(uint3 TID : SV_GroupThreadID) { | ||
| fn1(SharedData); | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| // RUN: %dxc -E main -T cs_6_3 -HV 202x -fcgl %s | FileCheck %s | ||
|
|
||
| struct Shared { | ||
| int A; | ||
| float F; | ||
| double Arr[4]; | ||
| }; | ||
|
|
||
| groupshared Shared SharedData; | ||
|
|
||
| // CHECK-LABEL: @"\01?fn1@@YAXAGAUShared@@@Z" | ||
| // CHECK: [[D:%.*]] = alloca double, align 8 | ||
| // CHECK: [[A:%.*]] = getelementptr inbounds %struct.Shared, %struct.Shared addrspace(3)* %Sh, i32 0, i32 0 | ||
| // CHECK: store i32 10, i32 addrspace(3)* [[A]], align 4 | ||
| // CHECK: [[F:%.*]] = getelementptr inbounds %struct.Shared, %struct.Shared addrspace(3)* %Sh, i32 0, i32 1 | ||
| // CHECK: store float 0x40263851E0000000, float addrspace(3)* %F, align 4 | ||
| // CHECK: store double 1.000000e+01, double* [[D]], align 8 | ||
| // CHECK: [[Z:%.*]] = load double, double* [[D]], align 8 | ||
| // CHECK: [[Arr:%.*]] = getelementptr inbounds %struct.Shared, %struct.Shared addrspace(3)* %Sh, i32 0, i32 2 | ||
| // CHECK: [[ArrIdx:%.*]] = getelementptr inbounds [4 x double], [4 x double] addrspace(3)* [[Arr]], i32 0, i32 1 | ||
| // CHECK: store double [[Z]], double addrspace(3)* [[ArrIdx]], align 4 | ||
| void fn1(groupshared Shared Sh) { | ||
| Sh.A = 10; | ||
| Sh.F = 11.11; | ||
| double D = 10.0; | ||
| Sh.Arr[1] = D; | ||
| } | ||
|
|
||
| [numthreads(4, 1, 1)] | ||
| void main(uint3 TID : SV_GroupThreadID) { | ||
| fn1(SharedData); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if it would be good to start a habit of linking the PR with the note? I like the idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a preference although I'd be inclined to follow the seeming convention of not including, unless you can find further support for this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of the items link issues. Some link PRs (the highlights for 1.8.2502). Others do nothing. Just seems random.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok i guess the ones i looked at didn't link, but i must have missed some.