Skip to content

Commit 2360804

Browse files
Erfan-AhmadiErfan-Ahmadi
committed
more shader cleanups and resolving PR issues
1 parent a4ce36e commit 2360804

File tree

10 files changed

+73
-105
lines changed

10 files changed

+73
-105
lines changed

‎include/nbl/asset/ICPUShader.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ class NBL_API ICPUShader : public IAsset, public IShader
4343
std::string&& filepathHint)
4444
: ICPUShader(core::make_smart_refctd_ptr<ICPUBuffer>(strlen(code) + 1u), stage, contentType, std::move(filepathHint))
4545
{
46+
assert(contentType != E_CONTENT_TYPE::ECT_SPIRV); // because using strlen needs `code` to be null-terminated
4647
memcpy(m_code->getPointer(), code, m_code->getSize());
4748
}
4849

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ namespace nbl::asset
1919
, m_GLSLCompiler(core::make_smart_refctd_ptr<CGLSLCompiler>(core::smart_refctd_ptr(sys)))
2020
{}
2121

22-
core::smart_refctd_ptr<ICPUBuffer> compileToSPIRV(const asset::ICPUShader* shader, const IShaderCompiler::SOptions& options);
22+
core::smart_refctd_ptr<const ICPUShader> compileToSPIRV(const asset::ICPUShader* shader, const IShaderCompiler::SOptions& options);
2323

2424
inline core::smart_refctd_ptr<IShaderCompiler> getShaderCompiler(IShader::E_CONTENT_TYPE contentType) const
2525
{

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

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -42,11 +42,10 @@ class NBL_API2 CGLSLCompiler final : public IShaderCompiler
4242
4343
@returns Shader containing SPIR-V bytecode.
4444
*/
45-
core::smart_refctd_ptr<ICPUBuffer> compileToSPIRV(const char* code, const CGLSLCompiler::SOptions& options) const;
4645

47-
core::smart_refctd_ptr<ICPUShader> createSPIRVShader(const char* code, const CGLSLCompiler::SOptions& options) const;
46+
core::smart_refctd_ptr<ICPUShader> compileToSPIRV(const char* code, const CGLSLCompiler::SOptions& options) const;
4847

49-
core::smart_refctd_ptr<ICPUShader> createSPIRVShader(system::IFile* sourceFile, const CGLSLCompiler::SOptions& options) const;
48+
core::smart_refctd_ptr<ICPUShader> compileToSPIRV(system::IFile* sourceFile, const CGLSLCompiler::SOptions& options) const;
5049

5150
/*
5251
If original code contains #version specifier,

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

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -24,11 +24,9 @@ class NBL_API2 CHLSLCompiler final : public IShaderCompiler
2424
IShader::E_CONTENT_TYPE getCodeContentType() const override { return IShader::E_CONTENT_TYPE::ECT_HLSL; };
2525
};
2626

27-
core::smart_refctd_ptr<ICPUBuffer> compileToSPIRV(const char* code, const CHLSLCompiler::SOptions& options) const;
27+
core::smart_refctd_ptr<ICPUShader> compileToSPIRV(const char* code, const CHLSLCompiler::SOptions& options) const;
2828

29-
core::smart_refctd_ptr<ICPUShader> createSPIRVShader(const char* code, const CHLSLCompiler::SOptions& options) const;
30-
31-
core::smart_refctd_ptr<ICPUShader> createSPIRVShader(system::IFile* sourceFile, const CHLSLCompiler::SOptions& options) const;
29+
core::smart_refctd_ptr<ICPUShader> compileToSPIRV(system::IFile* sourceFile, const CHLSLCompiler::SOptions& options) const;
3230

3331
template<typename... Args>
3432
static core::smart_refctd_ptr<ICPUShader> createOverridenCopy(const ICPUShader* original, const char* fmt, Args... args)

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -118,15 +118,15 @@ class NBL_API2 IShaderCompiler : public core::IReferenceCounted
118118
@includeFinder Optional parameter; if not nullptr, it will resolve the includes in the code
119119
@maxSelfInclusionCount used only when includeFinder is not nullptr
120120
@genDebugInfo Requests compiler to generate debug info (most importantly objects' names).
121-
The engine, while running on OpenGL, won't be able to set push constants for shaders loaded as SPIR-V without debug info.
121+
Anything non-vulkan, basically you can't recover the names of original variables with CShaderIntrospector without debug info
122+
By variables we mean names of PC/SSBO/UBO blocks, as they're essentially instantiations of structs with custom packing.
122123
*/
123124
struct SOptions
124125
{
125126
IShader::E_SHADER_STAGE stage = IShader::E_SHADER_STAGE::ESS_UNKNOWN;
126127
E_SPIRV_VERSION targetSpirvVersion = E_SPIRV_VERSION::ESV_1_6;
127128
std::string_view entryPoint = nullptr;
128129
std::string_view sourceIdentifier = nullptr;
129-
std::string* outAssembly = nullptr;
130130
const ISPIRVOptimizer* spirvOptimizer = nullptr;
131131
system::logger_opt_ptr logger = nullptr;
132132
const CIncludeFinder* includeFinder = nullptr;

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

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,9 @@
66
using namespace nbl;
77
using namespace nbl::asset;
88

9-
core::smart_refctd_ptr<ICPUBuffer> CCompilerSet::compileToSPIRV(const asset::ICPUShader* shader, const IShaderCompiler::SOptions& options)
9+
core::smart_refctd_ptr<const ICPUShader> CCompilerSet::compileToSPIRV(const ICPUShader* shader, const IShaderCompiler::SOptions& options)
1010
{
11-
core::smart_refctd_ptr<ICPUBuffer> outSpirvShader;
11+
core::smart_refctd_ptr<const ICPUShader> outSpirvShader = nullptr;
1212
if (shader)
1313
{
1414
switch (shader->getContentType())
@@ -18,13 +18,13 @@ core::smart_refctd_ptr<ICPUBuffer> CCompilerSet::compileToSPIRV(const asset::ICP
1818
const char* code = reinterpret_cast<const char*>(shader->getContent()->getPointer());
1919
if (options.getCodeContentType() == IShader::E_CONTENT_TYPE::ECT_HLSL)
2020
{
21-
m_HLSLCompiler->compileToSPIRV(code, static_cast<const CHLSLCompiler::SOptions&>(options));
21+
outSpirvShader = m_HLSLCompiler->compileToSPIRV(code, static_cast<const CHLSLCompiler::SOptions&>(options));
2222
}
2323
else
2424
{
2525
CHLSLCompiler::SOptions hlslCompilerOptions = {};
2626
hlslCompilerOptions.setCommonData(options);
27-
m_HLSLCompiler->compileToSPIRV(code, hlslCompilerOptions);
27+
outSpirvShader = m_HLSLCompiler->compileToSPIRV(code, hlslCompilerOptions);
2828
}
2929
}
3030
break;
@@ -33,19 +33,19 @@ core::smart_refctd_ptr<ICPUBuffer> CCompilerSet::compileToSPIRV(const asset::ICP
3333
const char* code = reinterpret_cast<const char*>(shader->getContent()->getPointer());
3434
if (options.getCodeContentType() == IShader::E_CONTENT_TYPE::ECT_GLSL)
3535
{
36-
m_GLSLCompiler->compileToSPIRV(code, static_cast<const CGLSLCompiler::SOptions&>(options));
36+
outSpirvShader = m_GLSLCompiler->compileToSPIRV(code, static_cast<const CGLSLCompiler::SOptions&>(options));
3737
}
3838
else
3939
{
4040
CGLSLCompiler::SOptions glslCompilerOptions = {};
4141
glslCompilerOptions.setCommonData(options);
42-
m_GLSLCompiler->compileToSPIRV(code, glslCompilerOptions);
42+
outSpirvShader = m_GLSLCompiler->compileToSPIRV(code, glslCompilerOptions);
4343
}
4444
}
4545
break;
4646
case IShader::E_CONTENT_TYPE::ECT_SPIRV:
4747
{
48-
outSpirvShader = core::smart_refctd_ptr<ICPUBuffer>(const_cast<ICPUBuffer*>(shader->getContent()));
48+
outSpirvShader = core::smart_refctd_ptr<const ICPUShader>(shader);
4949
}
5050
break;
5151
}

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

Lines changed: 38 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -18,88 +18,65 @@ CGLSLCompiler::CGLSLCompiler(core::smart_refctd_ptr<system::ISystem>&& system)
1818
{
1919
}
2020

21-
core::smart_refctd_ptr<ICPUBuffer> CGLSLCompiler::compileToSPIRV(const char* code, const CGLSLCompiler::SOptions& options) const
21+
core::smart_refctd_ptr<ICPUShader> CGLSLCompiler::compileToSPIRV(const char* code, const CGLSLCompiler::SOptions& options) const
2222
{
23-
core::smart_refctd_ptr<ICPUBuffer> outSpirv = nullptr;
24-
if (code)
23+
if (!code)
2524
{
26-
core::smart_refctd_ptr<asset::ICPUShader> cpuShader;
27-
if (options.includeFinder != nullptr)
25+
options.logger.log("code is nullptr", system::ILogger::ELL_ERROR);
26+
return nullptr;
27+
}
28+
29+
if (options.entryPoint.compare("main") != 0)
30+
{
31+
options.logger.log("shaderc requires entry point to be \"main\" in GLSL", system::ILogger::ELL_ERROR);
32+
return nullptr;
33+
}
34+
35+
core::smart_refctd_ptr<asset::ICPUShader> cpuShader;
36+
if (options.includeFinder != nullptr)
37+
{
38+
cpuShader = resolveIncludeDirectives(code, options.stage, options.sourceIdentifier.data(), options.maxSelfInclusionCount, options.logger);
39+
if (cpuShader)
2840
{
29-
cpuShader = resolveIncludeDirectives(code, options.stage, options.sourceIdentifier, options.maxSelfInclusionCount, options.logger);
30-
if (cpuShader)
31-
{
32-
code = reinterpret_cast<const char*>(cpuShader->getContent()->getPointer());
33-
}
41+
code = reinterpret_cast<const char*>(cpuShader->getContent()->getPointer());
3442
}
43+
}
3544

36-
if (strcmp(options.entryPoint, "main") == 0)
37-
{
38-
shaderc::Compiler comp;
39-
shaderc::CompileOptions shadercOptions; //default options
40-
shadercOptions.SetTargetSpirv(static_cast<shaderc_spirv_version>(options.targetSpirvVersion));
41-
const shaderc_shader_kind stage = options.stage == IShader::ESS_UNKNOWN ? shaderc_glsl_infer_from_source : ESStoShadercEnum(options.stage);
42-
const size_t glsl_len = strlen(code);
43-
if (options.genDebugInfo)
44-
shadercOptions.SetGenerateDebugInfo();
45+
shaderc::Compiler comp;
46+
shaderc::CompileOptions shadercOptions; //default options
47+
shadercOptions.SetTargetSpirv(static_cast<shaderc_spirv_version>(options.targetSpirvVersion));
48+
const shaderc_shader_kind stage = options.stage == IShader::ESS_UNKNOWN ? shaderc_glsl_infer_from_source : ESStoShadercEnum(options.stage);
49+
const size_t glsl_len = strlen(code);
50+
if (options.genDebugInfo)
51+
shadercOptions.SetGenerateDebugInfo();
4552

46-
shaderc::AssemblyCompilationResult asm_res;
47-
shaderc::SpvCompilationResult bin_res;
48-
if (options.outAssembly)
49-
{
50-
asm_res = comp.CompileGlslToSpvAssembly(code, glsl_len, stage, options.sourceIdentifier ? options.sourceIdentifier : "", shadercOptions);
51-
options.outAssembly->resize(std::distance(asm_res.cbegin(), asm_res.cend()));
52-
memcpy(options.outAssembly->data(), asm_res.cbegin(), options.outAssembly->size());
53-
bin_res = comp.AssembleToSpv(options.outAssembly->data(), options.outAssembly->size(), shadercOptions);
54-
}
55-
else
56-
{
57-
bin_res = comp.CompileGlslToSpv(code, glsl_len, stage, options.sourceIdentifier ? options.sourceIdentifier : "", options.entryPoint, shadercOptions);
58-
}
53+
shaderc::SpvCompilationResult bin_res = comp.CompileGlslToSpv(code, glsl_len, stage, options.sourceIdentifier.data() ? options.sourceIdentifier.data() : "", options.entryPoint.data(), shadercOptions);
5954

60-
if (bin_res.GetCompilationStatus() == shaderc_compilation_status_success)
61-
{
62-
outSpirv = core::make_smart_refctd_ptr<ICPUBuffer>(std::distance(bin_res.cbegin(), bin_res.cend()) * sizeof(uint32_t));
63-
memcpy(outSpirv->getPointer(), bin_res.cbegin(), outSpirv->getSize());
55+
if (bin_res.GetCompilationStatus() == shaderc_compilation_status_success)
56+
{
57+
auto outSpirv = core::make_smart_refctd_ptr<ICPUBuffer>(std::distance(bin_res.cbegin(), bin_res.cend()) * sizeof(uint32_t));
58+
memcpy(outSpirv->getPointer(), bin_res.cbegin(), outSpirv->getSize());
6459

65-
if (options.spirvOptimizer)
66-
outSpirv = options.spirvOptimizer->optimize(outSpirv.get(), options.logger);
67-
}
68-
else
69-
{
70-
options.logger.log(bin_res.GetErrorMessage(), system::ILogger::ELL_ERROR);
71-
}
72-
}
73-
else
74-
{
75-
options.logger.log("shaderc requires entry point to be \"main\" in GLSL", system::ILogger::ELL_ERROR);
76-
}
60+
if (options.spirvOptimizer)
61+
outSpirv = options.spirvOptimizer->optimize(outSpirv.get(), options.logger);
62+
return core::make_smart_refctd_ptr<asset::ICPUShader>(std::move(outSpirv), options.stage, IShader::E_CONTENT_TYPE::ECT_SPIRV, options.sourceIdentifier.data());
7763
}
7864
else
7965
{
80-
options.logger.log("code is nullptr", system::ILogger::ELL_ERROR);
81-
}
82-
return outSpirv;
83-
}
84-
85-
core::smart_refctd_ptr<ICPUShader> CGLSLCompiler::createSPIRVShader(const char* code, const CGLSLCompiler::SOptions& options) const
86-
{
87-
auto spirv = compileToSPIRV(code, options);
88-
if (spirv)
89-
return core::make_smart_refctd_ptr<asset::ICPUShader>(std::move(spirv), options.stage, IShader::E_CONTENT_TYPE::ECT_GLSL, options.sourceIdentifier);
90-
else
66+
options.logger.log(bin_res.GetErrorMessage(), system::ILogger::ELL_ERROR);
9167
return nullptr;
68+
}
9269
}
9370

94-
core::smart_refctd_ptr<ICPUShader> CGLSLCompiler::createSPIRVShader(system::IFile* sourceFile, const CGLSLCompiler::SOptions& options) const
71+
core::smart_refctd_ptr<ICPUShader> CGLSLCompiler::compileToSPIRV(system::IFile* sourceFile, const CGLSLCompiler::SOptions& options) const
9572
{
9673
size_t fileSize = sourceFile->getSize();
9774
std::string code(fileSize, '\0');
9875

9976
system::IFile::success_t success;
10077
sourceFile->read(success, code.data(), 0, fileSize);
10178
if (success)
102-
return createSPIRVShader(code.c_str(), options);
79+
return compileToSPIRV(code.c_str(), options);
10380
else
10481
return nullptr;
10582
}

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

Lines changed: 8 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -18,38 +18,28 @@ CHLSLCompiler::CHLSLCompiler(core::smart_refctd_ptr<system::ISystem>&& system)
1818
{
1919
}
2020

21-
core::smart_refctd_ptr<ICPUBuffer> CHLSLCompiler::compileToSPIRV(const char* code, const CHLSLCompiler::SOptions& options) const
21+
core::smart_refctd_ptr<ICPUShader> CHLSLCompiler::compileToSPIRV(const char* code, const CHLSLCompiler::SOptions& options) const
2222
{
23-
core::smart_refctd_ptr<ICPUBuffer> outSpirv = nullptr;
24-
if (code)
25-
{
26-
// TODO: Use DXC
27-
}
28-
else
23+
if (!code)
2924
{
3025
options.logger.log("code is nullptr", system::ILogger::ELL_ERROR);
26+
return nullptr;
3127
}
32-
return outSpirv;
33-
}
3428

35-
core::smart_refctd_ptr<ICPUShader> CHLSLCompiler::createSPIRVShader(const char* code, const CHLSLCompiler::SOptions& options) const
36-
{
37-
auto spirv = compileToSPIRV(code, options);
38-
if (spirv)
39-
return core::make_smart_refctd_ptr<asset::ICPUShader>(std::move(spirv), options.stage, IShader::E_CONTENT_TYPE::ECT_HLSL, options.sourceIdentifier);
40-
else
41-
return nullptr;
29+
core::smart_refctd_ptr<ICPUBuffer> spirv = nullptr;
30+
// TODO: Use DXC
31+
return core::make_smart_refctd_ptr<asset::ICPUShader>(std::move(spirv), options.stage, IShader::E_CONTENT_TYPE::ECT_SPIRV, options.sourceIdentifier.data());
4232
}
4333

44-
core::smart_refctd_ptr<ICPUShader> CHLSLCompiler::createSPIRVShader(system::IFile* sourceFile, const CHLSLCompiler::SOptions& options) const
34+
core::smart_refctd_ptr<ICPUShader> CHLSLCompiler::compileToSPIRV(system::IFile* sourceFile, const CHLSLCompiler::SOptions& options) const
4535
{
4636
size_t fileSize = sourceFile->getSize();
4737
std::string code(fileSize, '\0');
4838

4939
system::IFile::success_t success;
5040
sourceFile->read(success, code.data(), 0, fileSize);
5141
if (success)
52-
return createSPIRVShader(code.c_str(), options);
42+
return compileToSPIRV(code.c_str(), options);
5343
else
5444
return nullptr;
5545
}

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

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -122,8 +122,10 @@ const CIntrospectionData* CShaderIntrospector::introspect(const ICPUShader* _sha
122122

123123
if (_shader->getContentType() == ICPUShader::E_CONTENT_TYPE::ECT_GLSL || _shader->getContentType() == ICPUShader::E_CONTENT_TYPE::ECT_HLSL)
124124
{
125-
// TODO: actually use code to create a ICPUShader or change the function signature for "compilerSet"
126-
compilerSet->compileToSPIRV(_shader, commonCompileOptions);
125+
auto compiledShader = compilerSet->compileToSPIRV(_shader, commonCompileOptions);
126+
if (!compiledShader)
127+
return nullptr;
128+
return introspectSPV(compiledShader.get());
127129
}
128130
else if (_shader->getContentType() == ICPUShader::E_CONTENT_TYPE::ECT_SPIRV)
129131
return introspectSPV(_shader);

‎src/nbl/video/CVulkanLogicalDevice.h

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -535,26 +535,27 @@ class CVulkanLogicalDevice final : public ILogicalDevice
535535
if (cpushader->getContentType() != asset::ICPUShader::E_CONTENT_TYPE::ECT_SPIRV)
536536
asset::IShader::insertDefines(code, m_physicalDevice->getExtraGLSLDefines());
537537

538-
core::smart_refctd_ptr<asset::ICPUBuffer> spirv;
538+
core::smart_refctd_ptr<const asset::ICPUShader> spirvShader;
539539

540540
if (cpushader->getContentType() == asset::ICPUShader::E_CONTENT_TYPE::ECT_HLSL)
541541
{
542-
// TODO: actually use code to create a ICPUShader (via non allocating CPUBuffer) or change the function signature for "compilerSet"
543542
// TODO: add specific HLSLCompiler::SOption params
544-
spirv = compilerSet->compileToSPIRV(cpushader.get(), commonCompileOptions);
543+
spirvShader = compilerSet->compileToSPIRV(cpushader.get(), commonCompileOptions);
545544
}
546545
else if (cpushader->getContentType() == asset::ICPUShader::E_CONTENT_TYPE::ECT_GLSL)
547546
{
548-
spirv = compilerSet->compileToSPIRV(cpushader.get(), commonCompileOptions);
547+
spirvShader = compilerSet->compileToSPIRV(cpushader.get(), commonCompileOptions);
549548
}
550-
else if (cpushader->getContentType() == asset::ICPUShader::E_CONTENT_TYPE::ECT_SPIRV)
549+
else
551550
{
552-
spirv = core::smart_refctd_ptr<asset::ICPUBuffer>(const_cast<asset::ICPUBuffer*>(cpushader->getContent()));
551+
spirvShader = compilerSet->compileToSPIRV(cpushader.get(), commonCompileOptions);
553552
}
554553

555-
if (!spirv)
554+
if (!spirvShader || !spirvShader->getContent())
556555
return nullptr;
557556

557+
auto spirv = spirvShader->getContent();
558+
558559
VkShaderModuleCreateInfo vk_createInfo = { VK_STRUCTURE_TYPE_SHADER_MODULE_CREATE_INFO };
559560
vk_createInfo.pNext = nullptr;
560561
vk_createInfo.flags = static_cast<VkShaderModuleCreateFlags>(0u); // reserved for future use by Vulkan

0 commit comments

Comments
 (0)