Skip to content

Add target attribute #63

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Add target attribute #63

wants to merge 2 commits into from

Conversation

exdal
Copy link
Contributor

@exdal exdal commented Apr 18, 2025

This pull request introduces a new attribute for ScopedStatement called target. Target attribute allows programmer to write target (and optionally, it's version) specific shader code. Here is how it looks:

[entry(frag)]
fn main()
{
        [target(spirv)]
        {
                // Do some spir-v operations thats only valid on spir-v.
        }
        
        // Contents of this scope will be discarded for targets before SPIR-V 1.5.
        [target(spirv, 150)]
        {
                // Do some spir-v operations thats only valid on spir-v 1.5.
        }

        [target(glsl)]
        {
                // ...
        }

        [target(gles)]
        {
                // ...
        }
}

@SirLynix
Copy link
Contributor

this sounds nice, but I have troubles to see some real-life scenarios where it would be useful as is, since most NZSL code should be portable, do you have any example?

Copy link

codecov bot commented Apr 18, 2025

Codecov Report

Attention: Patch coverage is 15.30612% with 83 lines in your changes missing coverage. Please review.

Project coverage is 76.49%. Comparing base (afc5a5f) to head (837bf5e).

Files with missing lines Patch % Lines
src/NZSL/Parser.cpp 7.84% 47 Missing ⚠️
src/NZSL/LangWriter.cpp 15.78% 16 Missing ⚠️
src/NZSL/GlslWriter.cpp 10.00% 9 Missing ⚠️
src/NZSL/SpirV/SpirvAstVisitor.cpp 11.11% 8 Missing ⚠️
include/NZSL/Ast/Compare.inl 50.00% 2 Missing ⚠️
include/NZSL/Lang/ErrorList.hpp 0.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #63      +/-   ##
==========================================
- Coverage   76.78%   76.49%   -0.29%     
==========================================
  Files         159      159              
  Lines       21835    21931      +96     
  Branches     6745     6790      +45     
==========================================
+ Hits        16765    16777      +12     
- Misses       4977     5061      +84     
  Partials       93       93              
Files with missing lines Coverage Δ
include/NZSL/Ast/Nodes.hpp 45.45% <ø> (ø)
include/NZSL/LangWriter.hpp 100.00% <ø> (ø)
src/NZSL/Ast/AstSerializer.cpp 77.02% <100.00%> (+0.04%) ⬆️
src/NZSL/Ast/Cloner.cpp 87.29% <100.00%> (+0.05%) ⬆️
src/NZSL/Lang/Errors.cpp 81.81% <ø> (ø)
include/NZSL/Lang/ErrorList.hpp 35.94% <0.00%> (-0.24%) ⬇️
include/NZSL/Ast/Compare.inl 67.50% <50.00%> (-0.12%) ⬇️
src/NZSL/SpirV/SpirvAstVisitor.cpp 83.84% <11.11%> (-0.68%) ⬇️
src/NZSL/GlslWriter.cpp 84.93% <10.00%> (-0.45%) ⬇️
src/NZSL/LangWriter.cpp 87.88% <15.78%> (-1.28%) ⬇️
... and 1 more

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update afc5a5f...837bf5e. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@exdal
Copy link
Contributor Author

exdal commented Apr 18, 2025

this sounds nice, but I have troubles to see some real-life scenarios where it would be useful as is, since most NZSL code should be portable, do you have any example?

I've been thinking of supporting interop features similar to slang's (specifically this section).

@exdal exdal marked this pull request as draft April 18, 2025 13:49
@SirLynix
Copy link
Contributor

SirLynix commented Apr 18, 2025

this sounds nice, but I have troubles to see some real-life scenarios where it would be useful as is, since most NZSL code should be portable, do you have any example?

I've been thinking of supporting interop features similar to slang's (specifically this section).

Sounds good to prepare the glsl/spir-v assembly feature.

Style question: should it stay a separate attribute or be merged with the cond attribute? Like

[cond(target(spirv))]
{
    // SPIR-V specific
}

Also, how to handle SPIR-V for OpenGL (with OpenGL 4.5) and SPIR-V for Vulkan?

@exdal
Copy link
Contributor Author

exdal commented Apr 18, 2025

Style question: should it stay a separate attribute or be merged with the cond attribute?

The cond indeed makes more sense, but it wouldn't persist when compiling NZSL -> NZSL due to it's nature. So I decided to introduce a new attribute. However this can be avoided if we handle target attribute as special case from rest of the expressions.

Also, how to handle SPIR-V for OpenGL (with OpenGL 4.5) and SPIR-V for Vulkan?

I am also thinking about this too.
For syntax, going back to my first answer; maybe(?):

[cond(target(spirv) && ???)]
{
    // GL SPIR-V specific
}

Where ??? is related to desired output instruction set of SPIR-V. Or maybe output instruction set can be determined by compiler with the information inside target scope, for example OpExtInst GLSL450 ...?

@SirLynix
Copy link
Contributor

The cond indeed makes more sense, but it wouldn't persist when compiling NZSL -> NZSL due to it's nature

It's more complicated than that, cond is removed only if the compiler can reduce it to a constant boolean, for example:
https://github.com/NazaraEngine/ShaderLang/blob/main/src/NZSL/Ast/SanitizeVisitor.cpp#L1427-L1440

If we decide to add a target intrinsic it's possible to resolve it only when sanitizing the AST inside the GLSL/SPIR-V writer, it's a bit more complicated than what you've done but it should be ok.

One other option is to not add a target intrinsic but add builtin options which would be used as cond(SpirV), options that would be not set (unresolved) until the GLSL/SPIRV writer step.

Or maybe output instruction set can be determined by compiler with the information inside target scope, for example OpExtInst GLSL450 ...?

I don't think this is possible, OpExtInst GLSL450 is also used for valid Vulkan SPIR-V.

So we should probably split the GLSL/SPIR-V targets from the GL/GL ES/Vulkan environment, we also could add a way to test if an extension is present, etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants