Skip to content

Commit 670f92f

Browse files
hekotahekota
committed
code review feedback - rearrange code, rename arg, fix typo
1 parent 333966b commit 670f92f

File tree

3 files changed

+63
-68
lines changed

3 files changed

+63
-68
lines changed

‎clang/lib/CodeGen/CGHLSLRuntime.cpp‎

Lines changed: 17 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -201,18 +201,24 @@ static void fillPackoffsetLayout(const HLSLBufferDecl *BufDecl,
201201
VarDecl *VD = dyn_cast<VarDecl>(D);
202202
if (!VD || VD->getType().getAddressSpace() != LangAS::hlsl_constant)
203203
continue;
204+
205+
if (!VD->hasAttrs()) {
206+
Layout.push_back(-1);
207+
continue;
208+
}
209+
204210
size_t Offset = -1;
205-
if (VD->hasAttrs()) {
206-
for (auto *Attr : VD->getAttrs()) {
207-
if (auto *POA = dyn_cast<HLSLPackOffsetAttr>(Attr)) {
208-
Offset = POA->getOffsetInBytes();
209-
} else if (auto *RBA = dyn_cast<HLSLResourceBindingAttr>(Attr)) {
210-
if (RBA->getRegisterType() ==
211-
HLSLResourceBindingAttr::RegisterType::C) {
212-
// size of constant buffer row is 16 bytes
213-
Offset = RBA->getSlotNumber() * CBufferRowSizeInBytes;
214-
}
215-
}
211+
for (auto *Attr : VD->getAttrs()) {
212+
if (auto *POA = dyn_cast<HLSLPackOffsetAttr>(Attr)) {
213+
Offset = POA->getOffsetInBytes();
214+
break;
215+
}
216+
auto *RBA = dyn_cast<HLSLResourceBindingAttr>(Attr);
217+
if (RBA &&
218+
RBA->getRegisterType() == HLSLResourceBindingAttr::RegisterType::C) {
219+
// size of constant buffer row is 16 bytes
220+
Offset = RBA->getSlotNumber() * CBufferRowSizeInBytes;
221+
break;
216222
}
217223
}
218224
Layout.push_back(Offset);

‎clang/lib/CodeGen/HLSLBufferLayoutBuilder.cpp‎

Lines changed: 40 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -53,22 +53,22 @@ namespace clang {
5353
namespace CodeGen {
5454

5555
// Creates a layout type for given struct with HLSL constant buffer layout
56-
// taking into account Packoffsets, if provided.
56+
// taking into account PackOffsets, if provided.
5757
// Previously created layout types are cached by CGHLSLRuntime.
5858
//
5959
// The function iterates over all fields of the StructType (including base
6060
// classes) and calls layoutField to converts each field to its corresponding
6161
// LLVM type and to calculate its HLSL constant buffer layout. Any embedded
6262
// structs (or arrays of structs) are converted to target layout types as well.
6363
//
64-
// When Packoffsets are specified the elements will be placed based on the
64+
// When PackOffsets are specified the elements will be placed based on the
6565
// user-specified offsets. Not all elements must have a packoffset/register(c#)
66-
// annotation though. For those that don't, the Packoffsets array will constain
66+
// annotation though. For those that don't, the PackOffsets array will contain
6767
// -1 value instead. These elements must be placed at the end of the layout
6868
// after all of the elements with specific offset.
6969
llvm::TargetExtType *HLSLBufferLayoutBuilder::createLayoutType(
7070
const RecordType *StructType,
71-
const llvm::SmallVector<int32_t> *Packoffsets) {
71+
const llvm::SmallVector<int32_t> *PackOffsets) {
7272

7373
// check if we already have the layout type for this struct
7474
if (llvm::TargetExtType *Ty =
@@ -94,63 +94,53 @@ llvm::TargetExtType *HLSLBufferLayoutBuilder::createLayoutType(
9494
"HLSL doesn't support multiple inheritance");
9595
RecordTypes.push_back(D->bases_begin()->getType()->getAs<RecordType>());
9696
}
97+
98+
unsigned FieldOffset;
99+
llvm::Type *FieldType;
100+
97101
while (!RecordTypes.empty()) {
98102
const RecordType *RT = RecordTypes.back();
99103
RecordTypes.pop_back();
100104

101105
for (const auto *FD : RT->getDecl()->fields()) {
102-
unsigned FieldOffset = UINT_MAX;
103-
llvm::Type *FieldType = nullptr;
104-
105-
if (Packoffsets) {
106-
// have packoffset/register(c#) annotations
107-
assert(Index < Packoffsets->size() &&
108-
"number of elements in layout struct does not match number of "
109-
"packoffset annotations");
110-
int PO = (*Packoffsets)[Index++];
111-
if (PO != -1) {
112-
if (!layoutField(FD, EndOffset, FieldOffset, FieldType, PO))
113-
return nullptr;
114-
} else {
115-
// No packoffset/register(cX) annotation on this field;
116-
// Delay the layout until after all of the other elements
117-
// annotated with packoffsets/register(cX) are processed.
118-
DelayLayoutFields.emplace_back(FD, LayoutElements.size());
119-
// reserve space for this field in the layout vector and elements list
120-
Layout.push_back(UINT_MAX);
121-
LayoutElements.push_back(nullptr);
122-
continue;
123-
}
124-
} else {
125-
if (!layoutField(FD, EndOffset, FieldOffset, FieldType))
106+
assert((!PackOffsets || Index < PackOffsets->size()) &&
107+
"number of elements in layout struct does not match number of "
108+
"packoffset annotations");
109+
110+
// No PackOffset info at all, or have a valid packoffset/register(c#)
111+
// annotations value -> layout the field.
112+
int PO = PackOffsets ? (*PackOffsets)[Index++] : -1;
113+
if (!PackOffsets || PO != -1) {
114+
if (!layoutField(FD, EndOffset, FieldOffset, FieldType, PO))
126115
return nullptr;
116+
Layout.push_back((unsigned)FieldOffset);
117+
LayoutElements.push_back(FieldType);
118+
continue;
127119
}
128-
129-
assert(FieldOffset != UINT_MAX && FieldType != nullptr);
130-
Layout.push_back((unsigned)FieldOffset);
131-
LayoutElements.push_back(FieldType);
120+
// Have PackOffset info, but there is no packoffset/register(cX)
121+
// annotation on this field. Delay the layout until after all of the
122+
// other elements with packoffsets/register(cX) are processed.
123+
DelayLayoutFields.emplace_back(FD, LayoutElements.size());
124+
// reserve space for this field in the layout vector and elements list
125+
Layout.push_back(UINT_MAX);
126+
LayoutElements.push_back(nullptr);
132127
}
133128
}
134129

135130
// process delayed layouts
136-
if (!DelayLayoutFields.empty()) {
137-
for (auto I : DelayLayoutFields) {
138-
const FieldDecl *FD = I.first;
139-
unsigned IndexInLayoutElements = I.second;
140-
// the first item in layout vector is size, so we need to offset the index
141-
// by 1
142-
unsigned IndexInLayout = IndexInLayoutElements + 1;
143-
assert(Layout[IndexInLayout] == UINT_MAX &&
144-
LayoutElements[IndexInLayoutElements] == nullptr);
145-
146-
unsigned FieldOffset = UINT_MAX;
147-
llvm::Type *FieldType = nullptr;
148-
if (!layoutField(FD, EndOffset, FieldOffset, FieldType))
149-
return nullptr;
150-
151-
Layout[IndexInLayout] = (unsigned)FieldOffset;
152-
LayoutElements[IndexInLayoutElements] = FieldType;
153-
}
131+
for (auto I : DelayLayoutFields) {
132+
const FieldDecl *FD = I.first;
133+
unsigned IndexInLayoutElements = I.second;
134+
// the first item in layout vector is size, so we need to offset the index
135+
// by 1
136+
unsigned IndexInLayout = IndexInLayoutElements + 1;
137+
assert(Layout[IndexInLayout] == UINT_MAX &&
138+
LayoutElements[IndexInLayoutElements] == nullptr);
139+
140+
if (!layoutField(FD, EndOffset, FieldOffset, FieldType))
141+
return nullptr;
142+
Layout[IndexInLayout] = (unsigned)FieldOffset;
143+
LayoutElements[IndexInLayoutElements] = FieldType;
154144
}
155145

156146
// set the size of the buffer

‎clang/lib/Sema/SemaHLSL.cpp‎

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1963,13 +1963,12 @@ void SemaHLSL::ActOnEndOfTranslationUnit(TranslationUnitDecl *TU) {
19631963

19641964
// Set HasValidPackoffset if any of the decls has a register(c#) annotation;
19651965
for (const Decl *VD : DefaultCBufferDecls) {
1966-
if (const HLSLResourceBindingAttr *RBA =
1967-
VD->getAttr<HLSLResourceBindingAttr>()) {
1968-
if (RBA->getRegisterType() ==
1969-
HLSLResourceBindingAttr::RegisterType::C) {
1970-
DefaultCBuffer->setHasValidPackoffset(true);
1971-
break;
1972-
}
1966+
const HLSLResourceBindingAttr *RBA =
1967+
VD->getAttr<HLSLResourceBindingAttr>();
1968+
if (RBA &&
1969+
RBA->getRegisterType() == HLSLResourceBindingAttr::RegisterType::C) {
1970+
DefaultCBuffer->setHasValidPackoffset(true);
1971+
break;
19731972
}
19741973
}
19751974

0 commit comments

Comments
 (0)