-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[HLSL] Support packoffset attribute in AST #89836
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
Changes from 2 commits
4d8c726
c356db6
2e406f5
228aca7
72df629
1e1a8ce
932e748
db23da0
81f1b39
4b6cc97
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 |
---|---|---|
|
@@ -7398,6 +7398,26 @@ The full documentation is available here: https://docs.microsoft.com/en-us/windo | |
}]; | ||
} | ||
|
||
def HLSLPackOffsetDocs : Documentation { | ||
let Category = DocCatFunction; | ||
let Content = [{ | ||
The packoffset attribute is used to change the layout of a cbuffer. | ||
Attribute spelling in HLSL is: ``packoffset(c[Subcomponent[.component]])``. | ||
hekota marked this conversation as resolved.
Show resolved
Hide resolved
|
||
A subcomponent is a register number, which is an integer. A component is in the form of [.xyzw]. | ||
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. What about 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. Updated. |
||
|
||
Here're packoffset examples with and without component: | ||
damyanp marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
.. code-block:: c++ | ||
|
||
cbuffer A { | ||
float3 a : packoffset(c0.y); | ||
float4 b : packoffset(c4); | ||
} | ||
|
||
The full documentation is available here: https://learn.microsoft.com/en-us/windows/win32/direct3dhlsl/dx-graphics-hlsl-variable-packoffset | ||
}]; | ||
} | ||
|
||
def HLSLSV_DispatchThreadIDDocs : Documentation { | ||
let Category = DocCatFunction; | ||
let Content = [{ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -183,6 +183,86 @@ void Parser::ParseHLSLAnnotations(ParsedAttributes &Attrs, | |
return; | ||
} | ||
} break; | ||
case ParsedAttr::AT_HLSLPackOffset: { | ||
// Parse 'packoffset( c[Subcomponent][.component] )'. | ||
// Check '('. | ||
if (ExpectAndConsume(tok::l_paren, diag::err_expected_lparen_after)) { | ||
SkipUntil(tok::r_paren, StopAtSemi); // skip through ) | ||
return; | ||
} | ||
// Check c[Subcomponent] as an identifier. | ||
if (!Tok.is(tok::identifier)) { | ||
Diag(Tok.getLocation(), diag::err_expected) << tok::identifier; | ||
SkipUntil(tok::r_paren, StopAtSemi); // skip through ) | ||
return; | ||
} | ||
StringRef OffsetStr = Tok.getIdentifierInfo()->getName(); | ||
SourceLocation OffsetLoc = Tok.getLocation(); | ||
if (OffsetStr[0] != 'c') { | ||
Diag(Tok.getLocation(), diag::err_hlsl_packoffset_invalid_reg) | ||
<< OffsetStr; | ||
SkipUntil(tok::r_paren, StopAtSemi); // skip through ) | ||
return; | ||
} | ||
OffsetStr = OffsetStr.substr(1); | ||
unsigned SubComponent = 0; | ||
if (!OffsetStr.empty()) { | ||
// Make sure SubComponent is a number. | ||
if (OffsetStr.getAsInteger(10, SubComponent)) { | ||
Diag(OffsetLoc.getLocWithOffset(1), | ||
diag::err_hlsl_unsupported_register_number); | ||
hekota marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return; | ||
} | ||
} | ||
unsigned Component = 0; | ||
ConsumeToken(); // consume identifier. | ||
if (Tok.is(tok::period)) { | ||
ConsumeToken(); // consume period. | ||
if (!Tok.is(tok::identifier)) { | ||
Diag(Tok.getLocation(), diag::err_expected) << tok::identifier; | ||
SkipUntil(tok::r_paren, StopAtSemi); // skip through ) | ||
return; | ||
} | ||
StringRef ComponentStr = Tok.getIdentifierInfo()->getName(); | ||
SourceLocation SpaceLoc = Tok.getLocation(); | ||
ConsumeToken(); // consume identifier. | ||
// Make sure Component is a single character. | ||
if (ComponentStr.size() != 1) { | ||
Diag(SpaceLoc, diag::err_hlsl_unsupported_component) << ComponentStr; | ||
SkipUntil(tok::r_paren, StopAtSemi); // skip through ) | ||
return; | ||
} else { | ||
hekota marked this conversation as resolved.
Show resolved
Hide resolved
|
||
switch (ComponentStr[0]) { | ||
case 'x': | ||
Component = 0; | ||
break; | ||
case 'y': | ||
Component = 1; | ||
break; | ||
case 'z': | ||
Component = 2; | ||
break; | ||
case 'w': | ||
Component = 3; | ||
break; | ||
default: | ||
Diag(SpaceLoc, diag::err_hlsl_unsupported_component) << ComponentStr; | ||
SkipUntil(tok::r_paren, StopAtSemi); // skip through ) | ||
return; | ||
} | ||
} | ||
} | ||
unsigned Offset = SubComponent * 4 + Component; | ||
ASTContext &Ctx = Actions.getASTContext(); | ||
ArgExprs.push_back(IntegerLiteral::Create( | ||
Ctx, llvm::APInt(Ctx.getTypeSize(Ctx.getSizeType()), Offset), | ||
Ctx.getSizeType(), | ||
SourceLocation())); // Dummy location for integer literal. | ||
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. Why not give this the location of the parsed identifier? 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. Updated. |
||
if (ExpectAndConsume(tok::r_paren, diag::err_expected)) { | ||
SkipUntil(tok::r_paren, StopAtSemi); // skip through ) | ||
return; | ||
} | ||
} break; | ||
case ParsedAttr::UnknownAttribute: | ||
Diag(Loc, diag::err_unknown_hlsl_semantic) << II; | ||
return; | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -7314,6 +7314,47 @@ static void handleHLSLSV_DispatchThreadIDAttr(Sema &S, Decl *D, | |||||
D->addAttr(::new (S.Context) HLSLSV_DispatchThreadIDAttr(S.Context, AL)); | ||||||
} | ||||||
|
||||||
static void handleHLSLPackOffsetAttr(Sema &S, Decl *D, const ParsedAttr &AL) { | ||||||
if (!isa<VarDecl>(D)) { | ||||||
S.Diag(AL.getLoc(), diag::err_hlsl_attr_invalid_ast_node) | ||||||
<< AL << "cbuffer constant"; | ||||||
return; | ||||||
} | ||||||
auto *BufDecl = dyn_cast<HLSLBufferDecl>(D->getDeclContext()); | ||||||
if (!BufDecl) { | ||||||
S.Diag(AL.getLoc(), diag::err_hlsl_attr_invalid_ast_node) | ||||||
<< AL << "cbuffer constant"; | ||||||
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. Maybe:
Suggested change
"cbuffer constant" is less clear to me because the variable declaration isn't actually marked constant in the source, it is just implicitly constant because it is in a cbuffer. 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. Updated. |
||||||
return; | ||||||
hekota marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
} | ||||||
|
||||||
uint32_t Offset; | ||||||
if (!checkUInt32Argument(S, AL, AL.getArgAsExpr(0), Offset)) | ||||||
return; | ||||||
|
||||||
QualType T = cast<VarDecl>(D)->getType().getCanonicalType(); | ||||||
// Check if T is an array or struct type. | ||||||
// TODO: mark matrix type as aggregate type. | ||||||
bool IsAggregateTy = (T->isArrayType() || T->isStructureType()); | ||||||
|
||||||
unsigned ComponentNum = Offset & 0x3; | ||||||
// Check ComponentNum is valid for T. | ||||||
if (IsAggregateTy) { | ||||||
if (ComponentNum != 0) { | ||||||
S.Diag(AL.getLoc(), diag::err_hlsl_packoffset_cross_reg_boundary); | ||||||
return; | ||||||
} | ||||||
} else { | ||||||
unsigned size = S.getASTContext().getTypeSize(T); | ||||||
// Make sure ComponentNum + sizeof(T) <= 4. | ||||||
if ((ComponentNum * 32 + size) > 128) { | ||||||
S.Diag(AL.getLoc(), diag::err_hlsl_packoffset_cross_reg_boundary); | ||||||
return; | ||||||
} | ||||||
} | ||||||
|
||||||
D->addAttr(::new (S.Context) HLSLPackOffsetAttr(S.Context, AL, Offset)); | ||||||
} | ||||||
|
||||||
static void handleHLSLShaderAttr(Sema &S, Decl *D, const ParsedAttr &AL) { | ||||||
StringRef Str; | ||||||
SourceLocation ArgLoc; | ||||||
|
@@ -9735,6 +9776,9 @@ ProcessDeclAttribute(Sema &S, Scope *scope, Decl *D, const ParsedAttr &AL, | |||||
case ParsedAttr::AT_HLSLSV_DispatchThreadID: | ||||||
handleHLSLSV_DispatchThreadIDAttr(S, D, AL); | ||||||
break; | ||||||
case ParsedAttr::AT_HLSLPackOffset: | ||||||
handleHLSLPackOffsetAttr(S, D, AL); | ||||||
break; | ||||||
case ParsedAttr::AT_HLSLShader: | ||||||
handleHLSLShaderAttr(S, D, AL); | ||||||
break; | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
// RUN: %clang_cc1 -triple dxil-unknown-shadermodel6.3-library -S -finclude-default-header -ast-dump -x hlsl %s | FileCheck %s | ||
|
||
|
||
// CHECK: HLSLBufferDecl {{.*}} cbuffer A | ||
cbuffer A | ||
{ | ||
// CHECK-NEXT: VarDecl {{.*}} C1 'float4' | ||
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. We could use some tests for 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. Updated. |
||
// CHECK-NEXT: HLSLPackOffsetAttr {{.*}} 0 | ||
float4 C1 : packoffset(c); | ||
// CHECK-NEXT: VarDecl {{.*}} col:11 C2 'float' | ||
// CHECK-NEXT: HLSLPackOffsetAttr {{.*}} 4 | ||
float C2 : packoffset(c1); | ||
// CHECK-NEXT: VarDecl {{.*}} col:11 C3 'float' | ||
// CHECK-NEXT: HLSLPackOffsetAttr {{.*}} 5 | ||
float C3 : packoffset(c1.y); | ||
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. How about some valid cases for vector and array cbuffer elements? 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. Updated. |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
// RUN: %clang_cc1 -finclude-default-header -triple dxil-pc-shadermodel6.3-library -verify %s | ||
|
||
// expected-error@+1{{cannot mix packoffset elements with nonpackoffset elements in a cbuffer}} | ||
cbuffer Mix | ||
{ | ||
float4 M1 : packoffset(c0); | ||
float M2; | ||
float M3 : packoffset(c1.y); | ||
} | ||
|
||
// expected-error@+1{{cannot mix packoffset elements with nonpackoffset elements in a cbuffer}} | ||
cbuffer Mix2 | ||
{ | ||
float4 M4; | ||
float M5 : packoffset(c1.y); | ||
float M6 ; | ||
} | ||
|
||
// expected-error@+1{{attribute 'packoffset' only applies to cbuffer constant}} | ||
float4 g : packoffset(c0); | ||
|
||
cbuffer IllegalOffset | ||
{ | ||
// expected-error@+1{{invalid resource class specifier 't2' for packoffset, expected 'c'}} | ||
float4 i1 : packoffset(t2); | ||
// expected-error@+1{{invalid component 'm' used; expected 'x', 'y', 'z', or 'w'}} | ||
float i2 : packoffset(c1.m); | ||
} | ||
|
||
cbuffer Overlap | ||
{ | ||
float4 o1 : packoffset(c0); | ||
// expected-error@+1{{packoffset overlap between 'o2', 'o1'}} | ||
float2 o2 : packoffset(c0.z); | ||
} | ||
|
||
cbuffer CrossReg | ||
{ | ||
// expected-error@+1{{packoffset cannot cross register boundary}} | ||
float4 c1 : packoffset(c0.y); | ||
// expected-error@+1{{packoffset cannot cross register boundary}} | ||
float2 c2 : packoffset(c1.w); | ||
} | ||
|
||
struct ST { | ||
float s; | ||
}; | ||
|
||
cbuffer Aggregate | ||
{ | ||
// expected-error@+1{{packoffset cannot cross register boundary}} | ||
ST A1 : packoffset(c0.y); | ||
// expected-error@+1{{packoffset cannot cross register boundary}} | ||
float A2[2] : packoffset(c1.w); | ||
} | ||
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. Some additional error cases to consider:
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. Updated. |
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.
Looking at this more this seems really weird to me. The argument isn't actually an integer, it's a special string. You're parsing it to an integer, but that actually loses some fidelity.
We should probably store index and sub-component values in the attribute rather than collapsing it down into one value.
Uh oh!
There was an error while loading. Please reload this page.
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.
Do you mind the index and sub-component value store as integer or string?
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.
Information lost is the difference between 'c0' and 'c', 'xyzw' and 'rgba'.
If we want to keep that, they'll need to be kept these as string.
Then we'll need to parse the string when using the value as integer.
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 think it is fine to keep these as integers.
Uh oh!
There was an error while loading. Please reload this page.
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.
The feedback here still isn't resolved. The argument is an IdentifierArgument with special parsing handling, it isn't an integer. We can store the parsed integer values in the attribute but we shouldn't be calling the argument an IntArgument.
Of if we do, we should have separate int arguments for the row and column offsets.
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.
Updated.