Skip to content

Commit ff7ed06

Browse files
alichraghialichraghi
committed
review fixes
Signed-off-by: Ali Cheraghi <alichraghi@proton.me>
1 parent b2927ba commit ff7ed06

File tree

3 files changed

+31
-28
lines changed

3 files changed

+31
-28
lines changed

‎include/nbl/asset/utils/IShaderCompiler.h

Lines changed: 19 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -239,11 +239,10 @@ class NBL_API2 IShaderCompiler : public core::IReferenceCounted
239239
return true;
240240
}
241241

242-
std::string sourceIdentifier;
243-
244242
private:
245243
friend class SCompilerArgs;
246244
friend class SEntry;
245+
friend class CCache;
247246
friend void to_json(nlohmann::json&, const SPreprocessorArgs&);
248247
friend void from_json(const nlohmann::json&, SPreprocessorArgs&);
249248

@@ -264,9 +263,10 @@ class NBL_API2 IShaderCompiler : public core::IReferenceCounted
264263
// Sort them so equality and hashing are well defined
265264
std::sort(extraDefines.begin(), extraDefines.end(), [](const SMacroDefinition& lhs, const SMacroDefinition& rhs) {return lhs.identifier < rhs.identifier; });
266265
};
266+
std::string sourceIdentifier;
267267
std::vector<SMacroDefinition> extraDefines;
268268
};
269-
// TODO: SPreprocessorArgs could just be folded into `SCompilerArgs` to have less classes and operators
269+
// TODO: SPreprocessorArgs could just be folded into `SCompilerArgs` to have less classes and decompressShader
270270
struct SCompilerArgs final
271271
{
272272
public:
@@ -283,11 +283,9 @@ class NBL_API2 IShaderCompiler : public core::IReferenceCounted
283283
return retVal;
284284
}
285285

286-
IShader::E_SHADER_STAGE stage;
287-
SPreprocessorArgs preprocessorArgs;
288-
289286
private:
290287
friend class SEntry;
288+
friend class CCache;
291289
friend void to_json(nlohmann::json&, const SCompilerArgs&);
292290
friend void from_json(const nlohmann::json&, SCompilerArgs&);
293291

@@ -310,9 +308,11 @@ class NBL_API2 IShaderCompiler : public core::IReferenceCounted
310308
}
311309
}
312310

311+
IShader::E_SHADER_STAGE stage;
313312
E_SPIRV_VERSION targetSpirvVersion;
314313
std::vector<ISPIRVOptimizer::E_OPTIMIZER_PASS> optimizerPasses;
315314
core::bitflag<E_DEBUG_INFO_FLAGS> debugInfoFlags;
315+
SPreprocessorArgs preprocessorArgs;
316316
};
317317

318318
// The ordering is important here, the dependencies MUST be added to the array IN THE ORDER THE PREPROCESSOR INCLUDED THEM!
@@ -354,20 +354,28 @@ class NBL_API2 IShaderCompiler : public core::IReferenceCounted
354354
lookupHash = std::hash<core::blake3_hash_t>{}(hash);
355355
}
356356

357+
// Making an entry to insert into write cache
358+
inline SEntry(const SEntry& other, dependency_container_t dependencies, core::smart_refctd_ptr<asset::ICPUBuffer> spirv,
359+
core::blake3_hash_t uncompressedContentHash, size_t uncompressedSize)
360+
: mainFileContents(other.mainFileContents), compilerArgs(other.compilerArgs), hash(other.hash),
361+
lookupHash(other.lookupHash), dependencies(dependencies), spirv(spirv),
362+
uncompressedContentHash(uncompressedContentHash), uncompressedSize(uncompressedSize) {}
363+
357364
// Needed to get the vector deserialization automatically
358365
inline SEntry() {}
359366

360367
// Making the copy constructor deep-copy everything but the shader
361-
inline SEntry(const SEntry& other)
362-
: mainFileContents(other.mainFileContents), compilerArgs(other.compilerArgs), hash(other.hash), lookupHash(other.lookupHash),
363-
dependencies(other.dependencies), spirv(other.spirv), uncompressedSize(other.uncompressedSize) {}
368+
inline SEntry(const SEntry& other)
369+
: mainFileContents(other.mainFileContents), compilerArgs(other.compilerArgs), hash(other.hash),
370+
lookupHash(other.lookupHash), dependencies(other.dependencies), spirv(other.spirv),
371+
uncompressedContentHash(other.uncompressedContentHash), uncompressedSize(other.uncompressedSize) {}
364372

365373
inline SEntry& operator=(SEntry& other) = delete;
366374
inline SEntry(SEntry&& other) = default;
367375
// Used for late initialization while looking up a cache, so as not to always initialize an entry even if caching was not requested
368376
inline SEntry& operator=(SEntry&& other) = default;
369377

370-
core::smart_refctd_ptr<ICPUShader> decodeShader() const;
378+
core::smart_refctd_ptr<ICPUShader> decompressShader() const;
371379

372380
// TODO: make some of these private
373381
std::string mainFileContents;
@@ -376,6 +384,7 @@ class NBL_API2 IShaderCompiler : public core::IReferenceCounted
376384
size_t lookupHash;
377385
dependency_container_t dependencies;
378386
core::smart_refctd_ptr<asset::ICPUBuffer> spirv;
387+
core::blake3_hash_t uncompressedContentHash;
379388
size_t uncompressedSize;
380389
};
381390

‎src/nbl/asset/utils/IShaderCompiler.cpp

Lines changed: 7 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ inline core::smart_refctd_ptr<ICPUShader> nbl::asset::IShaderCompiler::compileTo
4545
CCache::SEntry writeEntry = *found;
4646
options.writeCache->insert(std::move(writeEntry));
4747
}
48-
return found->decodeShader();
48+
return found->decompressShader();
4949
}
5050
}
5151

@@ -56,7 +56,6 @@ inline core::smart_refctd_ptr<ICPUShader> nbl::asset::IShaderCompiler::compileTo
5656
const_cast<ICPUBuffer*>(backingBuffer)->setContentHash(backingBuffer->computeContentHash());
5757
}
5858

59-
std::cout << "writeCache: " << options.writeCache << "\n";
6059
if (options.writeCache)
6160
{
6261
auto* spirvBuffer = retVal->getContent();
@@ -83,11 +82,7 @@ inline core::smart_refctd_ptr<ICPUShader> nbl::asset::IShaderCompiler::compileTo
8382
auto compressedSpirvBuffer = core::make_smart_refctd_ptr<ICPUBuffer>(propsSize + destLen);
8483
memcpy(compressedSpirvBuffer->getPointer(), compressedSpirv.data(), compressedSpirvBuffer->getSize());
8584

86-
entry.dependencies = std::move(dependencies);
87-
entry.spirv = std::move(compressedSpirvBuffer);
88-
entry.uncompressedSize = spirvBuffer->getSize();
89-
90-
options.writeCache->insert(std::move(entry));
85+
options.writeCache->insert(std::move(SEntry(entry, std::move(dependencies), std::move(compressedSpirvBuffer), spirvBuffer->getContentHash(), spirvBuffer->getSize())));
9186
}
9287
return retVal;
9388
}
@@ -294,7 +289,7 @@ auto IShaderCompiler::CIncludeFinder::tryIncludeGenerators(const std::string& in
294289

295290
core::smart_refctd_ptr<asset::ICPUShader> IShaderCompiler::CCache::find(const SEntry& mainFile, const IShaderCompiler::CIncludeFinder* finder) const
296291
{
297-
return find_impl(mainFile, finder)->decodeShader();
292+
return find_impl(mainFile, finder)->decompressShader();
298293
}
299294

300295
IShaderCompiler::CCache::EntrySet::const_iterator IShaderCompiler::CCache::find_impl(const SEntry& mainFile, const IShaderCompiler::CIncludeFinder* finder) const
@@ -346,7 +341,7 @@ core::smart_refctd_ptr<ICPUBuffer> IShaderCompiler::CCache::serialize() const
346341
sizes[i] = entry.spirv->getSize();
347342

348343
// And add the params to the shader creation parameters array
349-
shaderCreationParams.emplace_back(entry.compilerArgs.stage, IShader::E_CONTENT_TYPE::ECT_SPIRV, entry.compilerArgs.preprocessorArgs.sourceIdentifier.data(), sizes[i], shaderBufferSize);
344+
shaderCreationParams.emplace_back(entry.compilerArgs.stage, entry.compilerArgs.preprocessorArgs.sourceIdentifier.data(), sizes[i], shaderBufferSize);
350345
// Enlarge the shader buffer by the size of the current shader
351346
shaderBufferSize += sizes[i];
352347
i++;
@@ -423,9 +418,11 @@ core::smart_refctd_ptr<IShaderCompiler::CCache> IShaderCompiler::CCache::deseria
423418
return retVal;
424419
}
425420

426-
core::smart_refctd_ptr<ICPUShader> nbl::asset::IShaderCompiler::CCache::SEntry::decodeShader() const
421+
core::smart_refctd_ptr<ICPUShader> nbl::asset::IShaderCompiler::CCache::SEntry::decompressShader() const
427422
{
428423
auto uncompressedBuf = core::make_smart_refctd_ptr<ICPUBuffer>(uncompressedSize);
424+
uncompressedBuf->setContentHash(uncompressedContentHash);
425+
429426
size_t dstSize = uncompressedBuf->getSize();
430427
size_t srcSize = spirv->getSize() - LZMA_PROPS_SIZE;
431428
ELzmaStatus status;

‎src/nbl/asset/utils/shaderCompiler_serialization.h

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -133,13 +133,12 @@ inline void from_json(const json& j, SEntry::SPreprocessingDependency& dependenc
133133

134134
struct CPUShaderCreationParams {
135135
IShader::E_SHADER_STAGE stage;
136-
IShader::E_CONTENT_TYPE contentType; //I think this one could be skipped since it's always going to be SPIR-V
137136
std::string filepathHint;
138137
uint64_t codeByteSize = 0;
139138
uint64_t offset = 0; // Offset into the serialized .bin for the Cache where code starts
140139

141-
CPUShaderCreationParams(IShader::E_SHADER_STAGE _stage, IShader::E_CONTENT_TYPE _contentType, std::string_view _filepathHint, uint64_t _codeByteSize, uint64_t _offset)
142-
: stage(_stage), contentType(_contentType), filepathHint(_filepathHint), codeByteSize(_codeByteSize), offset(_offset)
140+
CPUShaderCreationParams(IShader::E_SHADER_STAGE _stage, std::string_view _filepathHint, uint64_t _codeByteSize, uint64_t _offset)
141+
: stage(_stage), filepathHint(_filepathHint), codeByteSize(_codeByteSize), offset(_offset)
143142
{}
144143

145144
CPUShaderCreationParams() {};
@@ -148,10 +147,8 @@ struct CPUShaderCreationParams {
148147
inline void to_json(json& j, const CPUShaderCreationParams& creationParams)
149148
{
150149
uint32_t stage = static_cast<uint32_t>(creationParams.stage);
151-
uint32_t contentType = static_cast<uint32_t>(creationParams.contentType);
152150
j = json{
153151
{ "stage", stage },
154-
{ "contentType", contentType },
155152
{ "filepathHint", creationParams.filepathHint },
156153
{ "codeByteSize", creationParams.codeByteSize },
157154
{ "offset", creationParams.offset },
@@ -160,14 +157,12 @@ inline void to_json(json& j, const CPUShaderCreationParams& creationParams)
160157

161158
inline void from_json(const json& j, CPUShaderCreationParams& creationParams)
162159
{
163-
uint32_t stage, contentType;
160+
uint32_t stage;
164161
j.at("stage").get_to(stage);
165-
j.at("contentType").get_to(contentType);
166162
j.at("filepathHint").get_to(creationParams.filepathHint);
167163
j.at("codeByteSize").get_to(creationParams.codeByteSize);
168164
j.at("offset").get_to(creationParams.offset);
169165
creationParams.stage = static_cast<IShader::E_SHADER_STAGE>(stage);
170-
creationParams.contentType = static_cast<IShader::E_CONTENT_TYPE>(stage);
171166
}
172167

173168
// Serialize SEntry, keeping some fields as extra serialization to keep them separate on disk
@@ -180,6 +175,7 @@ inline void to_json(json& j, const SEntry& entry)
180175
{ "hash", entry.hash.data },
181176
{ "lookupHash", entry.lookupHash },
182177
{ "dependencies", entry.dependencies },
178+
{ "uncompressedContentHash", entry.uncompressedContentHash.data },
183179
{ "uncompressedSize", entry.uncompressedSize },
184180
};
185181
}
@@ -191,6 +187,7 @@ inline void from_json(const json& j, SEntry& entry)
191187
j.at("hash").get_to(entry.hash.data);
192188
j.at("lookupHash").get_to(entry.lookupHash);
193189
j.at("dependencies").get_to(entry.dependencies);
190+
j.at("uncompressedContentHash").get_to(entry.uncompressedContentHash.data);
194191
j.at("uncompressedSize").get_to(entry.uncompressedSize);
195192
entry.spirv = nullptr;
196193
}

0 commit comments

Comments
 (0)