[shaderc] Add MSL version and Apple platform configurations - iOS / macOS #3213
Conversation
tools/shaderc/shaderc.cpp
Outdated
| @@ -1215,7 +1248,7 @@ namespace bgfx | |||
| { | |||
| preprocessor.setDefine(glslDefine); | |||
| } | |||
| char temp[256]; | |||
There was a problem hiding this comment.
Following the Boy Scout Rule in coding, which suggests always leaving the code a bit better than you found it, this change aims to elevate the quality of the line from good to excellent.
Considering the readers and future maintainers, aligning the existing macOS code path with the updated iOS one could be quite beneficial. It might seem a bit perplexing to readers why there's a divergence between them. Therefore, this code adjustment could potentially ease any cognitive load for those working with the code in the future.
There was a problem hiding this comment.
Following the Boy Scout Rule in coding, which suggests always leaving the code a bit better than you found it, this change aims to elevate the quality of the line from good to excellent.
The problem with that philosophy is that PR then becomes full of non-related things.
What I would prefer, is:
- focused fix/feature PR that doesn't touch things that are not relevant to PR
- and then cleanup PR that does improve code
Actually order could be also 2, 1, depending on things you want to cleanup.
This way you're not overwhelming reviewer with large PR. I mean this would be insta-accepted if PR was simpler. 🙂
There was a problem hiding this comment.
Thank you for sharing your preference. It's so lovely that you are totally up for the Boy Scout Rule and understand its critical value. I'll happily segregate the feature work from the clean up work.
tools/shaderc/shaderc.cpp
Outdated
| if (profile->lang == ShadingLang::SpirV) | ||
| { | ||
| preprocessor.setDefine("BGFX_SHADER_LANGUAGE_SPIRV=1"); | ||
| } |
There was a problem hiding this comment.
I've reorganized the code for better usability. Now that the iOS and macOS code paths share almost identical configurations, it's highly likely that changes in one will require corresponding adjustments in the other. With this in mind, I felt it was helpful to group these paths together, making it easier to locate related code.
Initially, I considered merging the two code paths, but I hesitated, thinking it might deviate too much from the current platform selection format. However, now that we're discussing it, what are your thoughts on this idea ? Here's the idea in code:
else if (0 == bx::strCmpI(platform, "ios") || 0 == bx::strCmpI(platform, "osx") )
{
if (0 == bx::strCmpI(platform, "ios")
{
preprocessor.setDefine("BX_PLATFORM_IOS=1");
}
else
{
preprocessor.setDefine("BX_PLATFORM_OSX=1");
}
if (profile->lang != ShadingLang::Metal)
{
preprocessor.setDefine(glslDefine);
}
char temp[32];
bx::snprintf(
temp
, sizeof(temp)
, "BGFX_SHADER_LANGUAGE_METAL=%d"
, (profile->lang == ShadingLang::Metal) ? profile->id : 0
);
preprocessor.setDefine(temp);
}On a related note, I wanted to ask: should we continue supporting GLSL on iOS/macOS with shaderc ? If not, I'm more than willing to remove the current if (profile->lang != ShadingLang::Metal) code path.
There was a problem hiding this comment.
shaderc should be able to cross-compile for other targets on any platform. This obviously is not true for HLSL, but that's due D3D compiler constraint of being Windows only, not design choice.
tools/shaderc/shaderc_metal.cpp
Outdated
| minor = 1; | ||
| return; | ||
| default: | ||
| BX_ASSERT(0, "Unknown MSL version requested. Returning 1.0 as default."); |
There was a problem hiding this comment.
Should remove assert, because this is tool. Should be warning/info into output that always fire in case of downgrade.
tools/shaderc/shaderc_metal.cpp
Outdated
| switch (version) | ||
| { | ||
| case 1000: | ||
| major = 1; |
There was a problem hiding this comment.
It looks like you could do this as:
major = version/1000;
minor = (version-major*1000)/100;
|
Also test your change by rebuilding all shaders with SPIRV and Metal with shaderc from master, and then switch to your change and rebuild it again and make sure nothing is changing. |
…ion options from: https://developer.apple.com/documentation/metal/mtllanguageversion?language=objc Add configuration options for MSL compiler based on MSL version and Platform Configure MSL->SPIR-V version configuration based on when ray tracing types become available Set default metal compiler option to be metal 1.2, which is the default version assigned in the current SPIRV-Cross being used
Are you able to provide instructions on how to perform the rebuild ? |
cd examples
make rebuild TARGET=5TARGET=5 is Metal, TARGET=7 is SPIRV. |
…rm_configuration_in_shaderc
Thanks a bunch! Comparing |
Then it LGTM. Let me know if you want me to merge now? |
Niice! Yes, it'll be great if you could merge it in when you get the chance🤠 I've also got the clean up pr ready to go in #3258 too. |
…ion options from: https://developer.apple.com/documentation/metal/mtllanguageversion?language=objc (bkaradzic#3213) Add configuration options for MSL compiler based on MSL version and Platform Configure MSL->SPIR-V version configuration based on when ray tracing types become available Set default metal compiler option to be metal 1.2, which is the default version assigned in the current SPIRV-Cross being used
The Metal Shading Language cross-compiler in
shadercis improved by being more specific with the parameters it uses to configure SPIRV-Cross for theSPIRV -> MSL. This provides a greater deal more flexibility for users targetting iOS vs macOS shaders, and provides platform specific MSL generation conditions as outlined by SPIRV-Cross.The MSL version selection fine tuning is now made available to users of shaderc through MSL profiles, similarly matching the approach taken for users wishing to compile different versions of SPIRV for specific target Vulkan versions.
A fix is made to the current version selection of MSL 1.0.0, as it is now a deprecated version. The minimum default has now been set to MSL 1.2, based on the MSL specification. The selected MSL default version of 1.2 in this PR nicely blends the lowest version that caters to macOS 10.12 and iOS 10 - both very old versions at this point in time - so it seemed like a good choice.
In order to get specific features of the different MSL versions, the SPIRV that is generated by glslang that is then fed into SPIRV-Cross will need to have its client spec updated accordingly. This is handled in this PR, with the mapping of SPIRV version to MSL being based on the feature sets of SPIRV for Vulkan which should map to features in MSL.. the resultant feature mapping is based on what SPIRV-Cross actually implements, so it could be a case that ymmv with some of the features.