[HLSL][NFC] Move packoffset validation to separate function and calculate offsets in bytes #121989
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…late offsets in bytes
hekota
force-pushed
the
packoffset-validation-refactor
branch
from
7b03138 to
f68ea5a
Compare
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: Helena Kotas (hekota) ChangesThere will be more changes coming in to Full diff: https://github.com/llvm/llvm-project/pull/121989.diff 2 Files Affected:
diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index bc5f0662adf8e9..f6ecf046a2c42e 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -4721,9 +4721,9 @@ def HLSLPackOffset: HLSLAnnotationAttr {
let Args = [IntArgument<"Subcomponent">, IntArgument<"Component">];
let Documentation = [HLSLPackOffsetDocs];
let AdditionalMembers = [{
- unsigned getOffset() {
- return subcomponent * 4 + component;
- }
+ unsigned getOffsetInBytes() {
+ return subcomponent * 16 + component * 4;
+ }
}];
}
diff --git a/clang/lib/Sema/SemaHLSL.cpp b/clang/lib/Sema/SemaHLSL.cpp
index 600c800029fd05..10386749235d96 100644
--- a/clang/lib/Sema/SemaHLSL.cpp
+++ b/clang/lib/Sema/SemaHLSL.cpp
@@ -164,18 +164,20 @@ Decl *SemaHLSL::ActOnStartBuffer(Scope *BufferScope, bool CBuffer,
return Result;
}
-// Calculate the size of a legacy cbuffer type based on
+// Calculate the size of a legacy cbuffer type in bytes based on
// https://learn.microsoft.com/en-us/windows/win32/direct3dhlsl/dx-graphics-hlsl-packing-rules
static unsigned calculateLegacyCbufferSize(const ASTContext &Context,
QualType T) {
unsigned Size = 0;
- constexpr unsigned CBufferAlign = 128;
+ constexpr unsigned CBufferAlign = 16;
if (const RecordType *RT = T->getAs<RecordType>()) {
const RecordDecl *RD = RT->getDecl();
for (const FieldDecl *Field : RD->fields()) {
QualType Ty = Field->getType();
unsigned FieldSize = calculateLegacyCbufferSize(Context, Ty);
- unsigned FieldAlign = 32;
+ // FIXME: This is not the correct alignment, it does not work for 16-bit
+ // types. See llvm/llvm-project#119641.
+ unsigned FieldAlign = 4;
if (Ty->isAggregateType())
FieldAlign = CBufferAlign;
Size = llvm::alignTo(Size, FieldAlign);
@@ -194,17 +196,19 @@ static unsigned calculateLegacyCbufferSize(const ASTContext &Context,
calculateLegacyCbufferSize(Context, VT->getElementType());
Size = ElementSize * ElementCount;
} else {
- Size = Context.getTypeSize(T);
+ Size = Context.getTypeSize(T) / 8;
}
return Size;
}
-void SemaHLSL::ActOnFinishBuffer(Decl *Dcl, SourceLocation RBrace) {
- auto *BufDecl = cast<HLSLBufferDecl>(Dcl);
- BufDecl->setRBraceLoc(RBrace);
-
- // Validate packoffset.
+// Validate packoffset:
+// - make sure if packoffset it used on all decls or none
+// - the packoffset ranges must not overlap
+static void validatePackoffset(Sema &S, HLSLBufferDecl *BufDecl) {
llvm::SmallVector<std::pair<VarDecl *, HLSLPackOffsetAttr *>> PackOffsetVec;
+
+ // Make sure the packoffset annotations are either on all declarations
+ // or on none.
bool HasPackOffset = false;
bool HasNonPackOffset = false;
for (auto *Field : BufDecl->decls()) {
@@ -219,33 +223,41 @@ void SemaHLSL::ActOnFinishBuffer(Decl *Dcl, SourceLocation RBrace) {
}
}
- if (HasPackOffset && HasNonPackOffset)
- Diag(BufDecl->getLocation(), diag::warn_hlsl_packoffset_mix);
-
- if (HasPackOffset) {
- ASTContext &Context = getASTContext();
- // Make sure no overlap in packoffset.
- // Sort PackOffsetVec by offset.
- std::sort(PackOffsetVec.begin(), PackOffsetVec.end(),
- [](const std::pair<VarDecl *, HLSLPackOffsetAttr *> &LHS,
- const std::pair<VarDecl *, HLSLPackOffsetAttr *> &RHS) {
- return LHS.second->getOffset() < RHS.second->getOffset();
- });
-
- for (unsigned i = 0; i < PackOffsetVec.size() - 1; i++) {
- VarDecl *Var = PackOffsetVec[i].first;
- HLSLPackOffsetAttr *Attr = PackOffsetVec[i].second;
- unsigned Size = calculateLegacyCbufferSize(Context, Var->getType());
- unsigned Begin = Attr->getOffset() * 32;
- unsigned End = Begin + Size;
- unsigned NextBegin = PackOffsetVec[i + 1].second->getOffset() * 32;
- if (End > NextBegin) {
- VarDecl *NextVar = PackOffsetVec[i + 1].first;
- Diag(NextVar->getLocation(), diag::err_hlsl_packoffset_overlap)
- << NextVar << Var;
- }
+ if (!HasPackOffset)
+ return;
+
+ if (HasNonPackOffset)
+ S.Diag(BufDecl->getLocation(), diag::warn_hlsl_packoffset_mix);
+
+ // Make sure there is no overlap in packoffset - sort PackOffsetVec by offset
+ // and compare adjacent values.
+ ASTContext &Context = S.getASTContext();
+ std::sort(PackOffsetVec.begin(), PackOffsetVec.end(),
+ [](const std::pair<VarDecl *, HLSLPackOffsetAttr *> &LHS,
+ const std::pair<VarDecl *, HLSLPackOffsetAttr *> &RHS) {
+ return LHS.second->getOffsetInBytes() <
+ RHS.second->getOffsetInBytes();
+ });
+ for (unsigned i = 0; i < PackOffsetVec.size() - 1; i++) {
+ VarDecl *Var = PackOffsetVec[i].first;
+ HLSLPackOffsetAttr *Attr = PackOffsetVec[i].second;
+ unsigned Size = calculateLegacyCbufferSize(Context, Var->getType());
+ unsigned Begin = Attr->getOffsetInBytes();
+ unsigned End = Begin + Size;
+ unsigned NextBegin = PackOffsetVec[i + 1].second->getOffsetInBytes();
+ if (End > NextBegin) {
+ VarDecl *NextVar = PackOffsetVec[i + 1].first;
+ S.Diag(NextVar->getLocation(), diag::err_hlsl_packoffset_overlap)
+ << NextVar << Var;
}
}
+}
+
+void SemaHLSL::ActOnFinishBuffer(Decl *Dcl, SourceLocation RBrace) {
+ auto *BufDecl = cast<HLSLBufferDecl>(Dcl);
+ BufDecl->setRBraceLoc(RBrace);
+
+ validatePackoffset(SemaRef, BufDecl);
SemaRef.PopDeclContext();
}
|
|
@llvm/pr-subscribers-hlsl Author: Helena Kotas (hekota) ChangesThere will be more changes coming in to Full diff: https://github.com/llvm/llvm-project/pull/121989.diff 2 Files Affected:
diff --git a/clang/include/clang/Basic/Attr.td b/clang/include/clang/Basic/Attr.td
index bc5f0662adf8e9..f6ecf046a2c42e 100644
--- a/clang/include/clang/Basic/Attr.td
+++ b/clang/include/clang/Basic/Attr.td
@@ -4721,9 +4721,9 @@ def HLSLPackOffset: HLSLAnnotationAttr {
let Args = [IntArgument<"Subcomponent">, IntArgument<"Component">];
let Documentation = [HLSLPackOffsetDocs];
let AdditionalMembers = [{
- unsigned getOffset() {
- return subcomponent * 4 + component;
- }
+ unsigned getOffsetInBytes() {
+ return subcomponent * 16 + component * 4;
+ }
}];
}
diff --git a/clang/lib/Sema/SemaHLSL.cpp b/clang/lib/Sema/SemaHLSL.cpp
index 600c800029fd05..10386749235d96 100644
--- a/clang/lib/Sema/SemaHLSL.cpp
+++ b/clang/lib/Sema/SemaHLSL.cpp
@@ -164,18 +164,20 @@ Decl *SemaHLSL::ActOnStartBuffer(Scope *BufferScope, bool CBuffer,
return Result;
}
-// Calculate the size of a legacy cbuffer type based on
+// Calculate the size of a legacy cbuffer type in bytes based on
// https://learn.microsoft.com/en-us/windows/win32/direct3dhlsl/dx-graphics-hlsl-packing-rules
static unsigned calculateLegacyCbufferSize(const ASTContext &Context,
QualType T) {
unsigned Size = 0;
- constexpr unsigned CBufferAlign = 128;
+ constexpr unsigned CBufferAlign = 16;
if (const RecordType *RT = T->getAs<RecordType>()) {
const RecordDecl *RD = RT->getDecl();
for (const FieldDecl *Field : RD->fields()) {
QualType Ty = Field->getType();
unsigned FieldSize = calculateLegacyCbufferSize(Context, Ty);
- unsigned FieldAlign = 32;
+ // FIXME: This is not the correct alignment, it does not work for 16-bit
+ // types. See llvm/llvm-project#119641.
+ unsigned FieldAlign = 4;
if (Ty->isAggregateType())
FieldAlign = CBufferAlign;
Size = llvm::alignTo(Size, FieldAlign);
@@ -194,17 +196,19 @@ static unsigned calculateLegacyCbufferSize(const ASTContext &Context,
calculateLegacyCbufferSize(Context, VT->getElementType());
Size = ElementSize * ElementCount;
} else {
- Size = Context.getTypeSize(T);
+ Size = Context.getTypeSize(T) / 8;
}
return Size;
}
-void SemaHLSL::ActOnFinishBuffer(Decl *Dcl, SourceLocation RBrace) {
- auto *BufDecl = cast<HLSLBufferDecl>(Dcl);
- BufDecl->setRBraceLoc(RBrace);
-
- // Validate packoffset.
+// Validate packoffset:
+// - make sure if packoffset it used on all decls or none
+// - the packoffset ranges must not overlap
+static void validatePackoffset(Sema &S, HLSLBufferDecl *BufDecl) {
llvm::SmallVector<std::pair<VarDecl *, HLSLPackOffsetAttr *>> PackOffsetVec;
+
+ // Make sure the packoffset annotations are either on all declarations
+ // or on none.
bool HasPackOffset = false;
bool HasNonPackOffset = false;
for (auto *Field : BufDecl->decls()) {
@@ -219,33 +223,41 @@ void SemaHLSL::ActOnFinishBuffer(Decl *Dcl, SourceLocation RBrace) {
}
}
- if (HasPackOffset && HasNonPackOffset)
- Diag(BufDecl->getLocation(), diag::warn_hlsl_packoffset_mix);
-
- if (HasPackOffset) {
- ASTContext &Context = getASTContext();
- // Make sure no overlap in packoffset.
- // Sort PackOffsetVec by offset.
- std::sort(PackOffsetVec.begin(), PackOffsetVec.end(),
- [](const std::pair<VarDecl *, HLSLPackOffsetAttr *> &LHS,
- const std::pair<VarDecl *, HLSLPackOffsetAttr *> &RHS) {
- return LHS.second->getOffset() < RHS.second->getOffset();
- });
-
- for (unsigned i = 0; i < PackOffsetVec.size() - 1; i++) {
- VarDecl *Var = PackOffsetVec[i].first;
- HLSLPackOffsetAttr *Attr = PackOffsetVec[i].second;
- unsigned Size = calculateLegacyCbufferSize(Context, Var->getType());
- unsigned Begin = Attr->getOffset() * 32;
- unsigned End = Begin + Size;
- unsigned NextBegin = PackOffsetVec[i + 1].second->getOffset() * 32;
- if (End > NextBegin) {
- VarDecl *NextVar = PackOffsetVec[i + 1].first;
- Diag(NextVar->getLocation(), diag::err_hlsl_packoffset_overlap)
- << NextVar << Var;
- }
+ if (!HasPackOffset)
+ return;
+
+ if (HasNonPackOffset)
+ S.Diag(BufDecl->getLocation(), diag::warn_hlsl_packoffset_mix);
+
+ // Make sure there is no overlap in packoffset - sort PackOffsetVec by offset
+ // and compare adjacent values.
+ ASTContext &Context = S.getASTContext();
+ std::sort(PackOffsetVec.begin(), PackOffsetVec.end(),
+ [](const std::pair<VarDecl *, HLSLPackOffsetAttr *> &LHS,
+ const std::pair<VarDecl *, HLSLPackOffsetAttr *> &RHS) {
+ return LHS.second->getOffsetInBytes() <
+ RHS.second->getOffsetInBytes();
+ });
+ for (unsigned i = 0; i < PackOffsetVec.size() - 1; i++) {
+ VarDecl *Var = PackOffsetVec[i].first;
+ HLSLPackOffsetAttr *Attr = PackOffsetVec[i].second;
+ unsigned Size = calculateLegacyCbufferSize(Context, Var->getType());
+ unsigned Begin = Attr->getOffsetInBytes();
+ unsigned End = Begin + Size;
+ unsigned NextBegin = PackOffsetVec[i + 1].second->getOffsetInBytes();
+ if (End > NextBegin) {
+ VarDecl *NextVar = PackOffsetVec[i + 1].first;
+ S.Diag(NextVar->getLocation(), diag::err_hlsl_packoffset_overlap)
+ << NextVar << Var;
}
}
+}
+
+void SemaHLSL::ActOnFinishBuffer(Decl *Dcl, SourceLocation RBrace) {
+ auto *BufDecl = cast<HLSLBufferDecl>(Dcl);
+ BufDecl->setRBraceLoc(RBrace);
+
+ validatePackoffset(SemaRef, BufDecl);
SemaRef.PopDeclContext();
}
|
hekota
commented
Jan 7, 2025
| @@ -219,33 +223,41 @@ void SemaHLSL::ActOnFinishBuffer(Decl *Dcl, SourceLocation RBrace) { | |||
| } | |||
| } | |||
|
|
|||
| if (HasPackOffset && HasNonPackOffset) | |||
There was a problem hiding this comment.
This is best to review with 'Hide whitespaces'
bob80905
reviewed
Jan 7, 2025
bob80905
approved these changes
Jan 8, 2025
bogner
approved these changes
Jan 9, 2025
BaiXilin
pushed a commit
to BaiXilin/llvm-fix-vnni-instr-types
that referenced
this pull request
Jan 12, 2025
…late offsets in bytes (llvm#121989) There will be more changes coming in to `SemaHLSL::ActOnFinishBuffer` so it would be good to move the packoffset validation out to a separate function. This change also unifies the units for cbuffer offset calculations to bytes.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There will be more changes coming in to
SemaHLSL::ActOnFinishBufferso it would be good to move the packoffset validation out to a separate function. This change also unifies the units for cbuffer offset calculations to bytes.