Simplify `CapabilitySet` Diagnostic Printing #4678

ArielG-NV · 2024-07-18T17:20:31Z

Fixes: #4675
Fixes: #4683
Fixes: #4443
Fixes: #4585
Fixes: #4172

Made the following changes:

All capability diagnostic printing logic tries to simplify before printing. This means that we do not print atoms which imply another atom.
Do not print the _ prefix part of atom names since it is misleading users on what they should use to solve a capability issue encountered. (_Internal External atom changes are not in this PR)
Bundle together printing of all sets which contain exactly the same atoms (excluding abstract atoms). This allows printing the following vertex/fragment/hull/domain/... + glsl instead of vertex + glsl | fragment + glsl | hull + glsl | domain + glsl | ....
Rework how entry-point errors are reported to users (example at bottom of PR comment)
Rework how atom-provenance data is collected to be leaner and more useful so we can rework the errors. There are 2 notable changes here:
- We no longer store a list which describes where the first of an CapabilityAtom comes from. This heavily simplifies AST logic for the capability system. AST parsing of capabilities is much faster. The trade-off is faster AST parsing and correct AST node data for slower diagnostics if an error is found
- atom-provenance data now stores a reference to an atom's use-site to provide information on where and what is wrong with user code versus only sharing what and not where.

case 1 of example output which needs to be addressed (test capabilitySimplification1.slang):

shader.hlsl(11): error 36107: entrypoint 'main' requires capability 'textualTarget + cuda + cuda_sm_1_0 + cuda_sm_2_0 + cuda_sm_3_0 + cuda_sm_3_5 + cuda_sm_4_0 + cuda_sm_5_0 + cuda_sm_6_0 + cuda_sm_7_0 + vertex | textualTarget + cuda + cuda_sm_1_0 + cuda_sm_2_0 + cuda_sm_3_0 + cuda_sm_3_5 + cuda_sm_4_0 + cuda_sm_5_0 + cuda_sm_6_0 + cuda_sm_7_0 + fragment | textualTarget + cuda + cuda_sm_1_0 + cuda_sm_2_0 + cuda_sm_3_0 + cuda_sm_3_5 + cuda_sm_4_0 + cuda_sm_5_0 + cuda_sm_6_0 + cuda_sm_7_0 + compute | textualTarget + cuda + cuda_sm_1_0 + cuda_sm_2_0 + cuda_sm_3_0 + cuda_sm_3_5 + cuda_sm_4_0 + cuda_sm_5_0 + cuda_sm_6_0 + cuda_sm_7_0 + hull | textualTarget + cuda + cuda_sm_1_0 + cuda_sm_2_0 + cuda_sm_3_0 + cuda_sm_3_5 + cuda_sm_4_0 + cuda_sm_5_0 + cuda_sm_6_0 + cuda_sm_7_0 + domain | textualTarget + cuda + cuda_sm_1_0 + cuda_sm_2_0 + cuda_sm_3_0 + cuda_sm_3_5 + cuda_sm_4_0 + cuda_sm_5_0 + cuda_sm_6_0 + cuda_sm_7_0 + geometry | textualTarget + cuda + cuda_sm_1_0 + cuda_sm_2_0 + cuda_sm_3_0 + cuda_sm_3_5 + cuda_sm_4_0 + cuda_sm_5_0 + cuda_sm_6_0 + cuda_sm_7_0 + raygen | textualTarget + cuda + cuda_sm_1_0 + cuda_sm_2_0 + cuda_sm_3_0 + cuda_sm_3_5 + cuda_sm_4_0 + cuda_sm_5_0 + cuda_sm_6_0 + cuda_sm_7_0 + intersection | textualTarget + cuda + cuda_sm_1_0 + cuda_sm_2_0 + cuda_sm_3_0 + cuda_sm_3_5 + cuda_sm_4_0 + cuda_sm_5_0 + cuda_sm_6_0 + cuda_sm_7_0 + anyhit | textualTarget + cuda + cuda_sm_1_0 + cuda_sm_2_0 + cuda_sm_3_0 + cuda_sm_3_5 + cuda_sm_4_0 + cuda_sm_5_0 + cuda_sm_6_0 + cuda_sm_7_0 + closesthit | textualTarget + cuda + cuda_sm_1_0 + cuda_sm_2_0 + cuda_sm_3_0 + cuda_sm_3_5 + cuda_sm_4_0 + cuda_sm_5_0 + cuda_sm_6_0 + cuda_sm_7_0 + miss | textualTarget + cuda + cuda_sm_1_0 + cuda_sm_2_0 + cuda_sm_3_0 + cuda_sm_3_5 + cuda_sm_4_0 + cuda_sm_5_0 + cuda_sm_6_0 + cuda_sm_7_0 + mesh | textualTarget + cuda + cuda_sm_1_0 + cuda_sm_2_0 + cuda_sm_3_0 + cuda_sm_3_5 + cuda_sm_4_0 + cuda_sm_5_0 + cuda_sm_6_0 + cuda_sm_7_0 + amplification | textualTarget + cuda + cuda_sm_1_0 + cuda_sm_2_0 + cuda_sm_3_0 + cuda_sm_3_5 + cuda_sm_4_0 + cuda_sm_5_0 + cuda_sm_6_0 + cuda_sm_7_0 + callable | textualTarget + hlsl + sm_4_0 + sm_4_1 + sm_5_0 + sm_5_1 + sm_6_0 + sm_6_1 + sm_6_2 + sm_6_3 + sm_6_4 + sm_6_5 + vertex | textualTarget + hlsl + sm_4_0 + sm_4_1 + sm_5_0 + sm_5_1 + sm_6_0 + sm_6_1 + sm_6_2 + sm_6_3 + sm_6_4 + sm_6_5 + fragment | textualTarget + hlsl + sm_4_0 + sm_4_1 + sm_5_0 + sm_5_1 + sm_6_0 + sm_6_1 + sm_6_2 + sm_6_3 + sm_6_4 + sm_6_5 + compute | textualTarget + hlsl + sm_4_0 + sm_4_1 + sm_5_0 + sm_5_1 + sm_6_0 + sm_6_1 + sm_6_2 + sm_6_3 + sm_6_4 + sm_6_5 + hull | textualTarget + hlsl + sm_4_0 + sm_4_1 + sm_5_0 + sm_5_1 + sm_6_0 + sm_6_1 + sm_6_2 + sm_6_3 + sm_6_4 + sm_6_5 + domain | textualTarget + hlsl + sm_4_0 + sm_4_1 + sm_5_0 + sm_5_1 + sm_6_0 + sm_6_1 + sm_6_2 + sm_6_3 + sm_6_4 + sm_6_5 + geometry | textualTarget + hlsl + sm_4_0 + sm_4_1 + sm_5_0 + sm_5_1 + sm_6_0 + sm_6_1 + sm_6_2 + sm_6_3 + sm_6_4 + sm_6_5 + raygen | textualTarget + hlsl + sm_4_0 + sm_4_1 + sm_5_0 + sm_5_1 + sm_6_0 + sm_6_1 + sm_6_2 + sm_6_3 + sm_6_4 + sm_6_5 + intersection | textualTarget + hlsl + sm_4_0 + sm_4_1 + sm_5_0 + sm_5_1 + sm_6_0 + sm_6_1 + sm_6_2 + sm_6_3 + sm_6_4 + sm_6_5 + anyhit | textualTarget + hlsl + sm_4_0 + sm_4_1 + sm_5_0 + sm_5_1 + sm_6_0 + sm_6_1 + sm_6_2 + sm_6_3 + sm_6_4 + sm_6_5 + closesthit | textualTarget + hlsl + sm_4_0 + sm_4_1 + sm_5_0 + sm_5_1 + sm_6_0 + sm_6_1 + sm_6_2 + sm_6_3 + sm_6_4 + sm_6_5 + miss | textualTarget + hlsl + sm_4_0 + sm_4_1 + sm_5_0 + sm_5_1 + sm_6_0 + sm_6_1 + sm_6_2 + sm_6_3 + sm_6_4 + sm_6_5 + mesh | textualTarget + hlsl + sm_4_0 + sm_4_1 + sm_5_0 + sm_5_1 + sm_6_0 + sm_6_1 + sm_6_2 + sm_6_3 + sm_6_4 + sm_6_5 + amplification | textualTarget + hlsl + sm_4_0 + sm_4_1 + sm_5_0 + sm_5_1 + sm_6_0 + sm_6_1 + sm_6_2 + sm_6_3 + sm_6_4 + sm_6_5 + callable', which is incompatible with the current compilation target 'spirv_1_0 + spirv_1_1 + spirv_1_2 + spirv_1_3 + spirv_1_4 + spirv_1_5 + fragment + SPV_KHR_fragment_shader_barycentric + SPV_EXT_fragment_fully_covered + SPV_EXT_shader_atomic_float_add + SPV_EXT_shader_atomic_float_min_max + SPV_EXT_mesh_shader + SPV_KHR_ray_tracing + SPV_KHR_ray_query + SPV_GOOGLE_user_type'.
uint4 main(int4 input0 : ATTR0, uint4 input1 : ATTR1) : SV_Target {
      ^~~~
hlsl.meta.slang(14867): note: see using of 'WaveMultiPrefixProduct'
hlsl.meta.slang(14867): note: see definition of 'WaveMultiPrefixProduct'
hlsl.meta.slang(14867): note: see using of 'WaveMultiPrefixProduct'
hlsl.meta.slang(14867): note: see definition of 'WaveMultiPrefixProduct'
hlsl.meta.slang(14867): note: see using of 'WaveMultiPrefixProduct'
hlsl.meta.slang(14867): note: see definition of 'WaveMultiPrefixProduct'
hlsl.meta.slang(14867): note: see using of 'WaveMultiPrefixProduct'
hlsl.meta.slang(14867): note: see definition of 'WaveMultiPrefixProduct'
hlsl.meta.slang(14867): note: see using of 'WaveMultiPrefixProduct'
hlsl.meta.slang(14867): note: see definition of 'WaveMultiPrefixProduct'
hlsl.meta.slang(14867): note: see using of 'WaveMultiPrefixProduct'
hlsl.meta.slang(14867): note: see definition of 'WaveMultiPrefixProduct'
hlsl.meta.slang(14867): note: see using of 'WaveMultiPrefixProduct'
hlsl.meta.slang(14867): note: see definition of 'WaveMultiPrefixProduct'
hlsl.meta.slang(14867): note: see using of 'WaveMultiPrefixProduct'
hlsl.meta.slang(14867): note: see definition of 'WaveMultiPrefixProduct'
hlsl.meta.slang(14867): note: see using of 'WaveMultiPrefixProduct'
hlsl.meta.slang(14867): note: see definition of 'WaveMultiPrefixProduct'
hlsl.meta.slang(14867): note: see using of 'WaveMultiPrefixProduct'
hlsl.meta.slang(14867): note: see definition of 'WaveMultiPrefixProduct'
hlsl.meta.slang(14867): note: see using of 'WaveMultiPrefixProduct'
hlsl.meta.slang(14867): note: see definition of 'WaveMultiPrefixProduct'

case 1 now:

7: entrypoint 'computeMain' does not support compilation target 'glsl' with stage 'compute'
void computeMain()
     ^~~~~~~~~~~
tests\language-feature\capability\capabilitySimplification3.slang(15): note: see using of 'WaveMultiPrefixProduct'
    WaveMultiPrefixProduct(1, 0);
    ^~~~~~~~~~~~~~~~~~~~~~
hlsl.meta.slang(15101): note: see definition of 'WaveMultiPrefixProduct'
T WaveMultiPrefixProduct(T value, uint4 mask)
  ^~~~~~~~~~~~~~~~~~~~~~
hlsl.meta.slang(15100): note: see declaration of 'require'
[require(cuda_hlsl, waveprefix)]
 ^~~~~~~

case 2 of what needs to be addressed (test capability7.slang):

tests/language-feature/capability/capability7.slang(26): error 36104: 'processDataBad' uses undeclared capability 'sm_5_0'.
void processDataBad()
     ^~~~~~~~~~~~~~
tests/language-feature/capability/capability7.slang(28): note: see using of 'processDataBadNested'
    processDataBadNested();
    ^~~~~~~~~~~~~~~~~~~~
tests/language-feature/capability/capability7.slang(20): note: see definition of 'processDataBadNested'
void processDataBadNested()
     ^~~~~~~~~~~~~~~~~~~~
tests/language-feature/capability/capability7.slang(26): error 36104: 'processDataBad' uses undeclared capability 'sm_5_1'.
void processDataBad()
     ^~~~~~~~~~~~~~
tests/language-feature/capability/capability7.slang(28): note: see using of 'processDataBadNested'
    processDataBadNested();
    ^~~~~~~~~~~~~~~~~~~~
tests/language-feature/capability/capability7.slang(20): note: see definition of 'processDataBadNested'
void processDataBadNested()
     ^~~~~~~~~~~~~~~~~~~~
...

case 2 now:

tests/language-feature/capability/capability7.slang(26): error 36104: 'processDataBad' uses undeclared capability 'sm_6_0'.
void processDataBad()
     ^~~~~~~~~~~~~~
tests/language-feature/capability/capability7.slang(28): note: see using of 'processDataBadNested'
    processDataBadNested();
    ^~~~~~~~~~~~~~~~~~~~
tests/language-feature/capability/capability7.slang(20): note: see definition of 'processDataBadNested'
void processDataBadNested()
     ^~~~~~~~~~~~~~~~~~~~

case 3 of what needs to be addressed (test capabilitySimplification2.slang):

tests\language-feature\capability\capabilitySimplification2.slang(25): warning 41012: entry point 'computeMain' uses additional capabilities that are not part of the specified profile 'sm_5_0'. The profile setting is automatically updated to include these capabilities: 'glsl + GLSL_130 + GLSL_140 + GLSL_330 + GLSL_400 + GLSL_430  + GLSL_450 + GL_EXT_nonuniform_qualifier + GL_KHR_shader_subgroup_arithmetic + GL_KHR_shader_subgroup_ballot + GL_KHR_shader_subgroup_quad + GL_KHR_shader_subgroup_shuffle + GL_KHR_shader_subgroup_vote'

case 3 now

tests\language-feature\capability\capabilitySimplification2.slang(25): warning 41012: entry point 'computeMain' uses additional capabilities that are not part of the specified profile 'sm_5_0'. The profile setting is automatically updated to include these capabilities: 'GLSL_450 + GL_EXT_nonuniform_qualifier + GL_KHR_shader_subgroup_arithmetic + GL_KHR_shader_subgroup_ballot + GL_KHR_shader_subgroup_quad + GL_KHR_shader_subgroup_shuffle + GL_KHR_shader_subgroup_vote'

Made the following changes: 1. All capability diagnostic printing logic tries to simplify before printing This means that we do not print atoms which imply another atom. 2. Don't print the `_` prefix part of atom names since it is misleading users on what they should use to solve a capability issue encountered.

csyonghe · 2024-07-18T18:25:27Z

I think there is another opportunity for simplification: when complaining that two caoapbility sets are incompatible, we can just print the "key" atoms in both sets instead of everything. Because only the key atoms are causing the incompatiblity.

ArielG-NV · 2024-07-18T18:51:24Z

I think there is another opportunity for simplification: when complaining that two capability sets are incompatible, we can just print the "key" atoms in both sets instead of everything. Because only the key atoms are causing the incompatibility.

100% agree, I think I will put this change in a separate PR more geared-towards solving #4443. The issue #4683

I think I will also add a method to print sets (where the only difference is a shader stage) in the form of "vertex/fragment {capabilities}". This would be quite useful since normally capabilities overlap across stages, This change will be in this PR since it touches the same code.

…rinting

…/github.com/ArielG-NV/slang into implied-atom-removal-from-diagnostic-prints

… text

jkwak-work

Looks good to me.

But the new message is still not clear to me of what is wrong.

csyonghe · 2024-07-19T06:02:52Z

source/slang/slang-diagnostic-defs.h

@@ -737,8 +737,8 @@ DIAGNOSTIC(41001, Error, recursiveType, "type '$0' contains cyclic reference to

 DIAGNOSTIC(41010, Warning, missingReturn, "control flow may reach end of non-'void' function")
 DIAGNOSTIC(41011, Error, profileIncompatibleWithTargetSwitch, "__target_switch has no compatable target with current profile '$0'")
-DIAGNOSTIC(41012, Warning, profileImplicitlyUpgraded, "user set `profile` had an implicit upgrade applied to it, atoms added: '$0'")
-DIAGNOSTIC(41012, Error, profileImplicitlyUpgradedRestrictive, "user set `profile` had an implicit upgrade applied to it, atoms added: '$0'")
+DIAGNOSTIC(41012, Warning, profileImplicitlyUpgraded, "user set profile $0 had an implicit upgrade applied to it, atoms added: '$1'")


"shader '$0' uses additional capabilities that are not part of the specified profile '$1'. The profile setting is automatically updated to include these capabilities: '$2'"

csyonghe · 2024-07-19T06:03:53Z

source/slang/slang-diagnostic-defs.h

-DIAGNOSTIC(41012, Warning, profileImplicitlyUpgraded, "user set `profile` had an implicit upgrade applied to it, atoms added: '$0'")
-DIAGNOSTIC(41012, Error, profileImplicitlyUpgradedRestrictive, "user set `profile` had an implicit upgrade applied to it, atoms added: '$0'")
+DIAGNOSTIC(41012, Warning, profileImplicitlyUpgraded, "user set profile $0 had an implicit upgrade applied to it, atoms added: '$1'")
+DIAGNOSTIC(41012, Error, profileImplicitlyUpgradedRestrictive, "user set profile $0 had an implicit upgrade applied to it, atoms added: '$1'")


"shader '$0' uses capabilities that are not part of the specified profile '$1'. Missing capabilities are: '$2'"

ArielG-NV · 2024-07-19T15:29:12Z

But the new message is still not clear to me of what is wrong.

This is a problem I should fix in this PR, otherwise this PR has no value in changing error/warning messages.
To fix clarity of error/warning diagnosis of entry-point incompatibility I plan to-do the following:

tell user the failing target+stage combo
diagnose based on stored capability provenance the exact function failing and where its definition is (this will change out the previous logic which just gave some rough approximation of where the problem was)
- The fix required for the provenance storage is likely to store atoms which cause a "loss of atoms" and "addition of atoms". Changing this will likely improve AST compile times as well (1-2%). The downside is that debug paths (printing errors) will be slower.

so what this means is the following code:

void myNestedNestedSafeCall()
{
    AllMemoryBarrier();
}

void myNestedNestedBadCall()
{
    AllMemoryBarrier();
    WaveMultiPrefixProduct(1, 0);
}

void myNestedCall()
{
    myNestedNestedSafeCall();
    myNestedNestedBadCall();
}

[numthreads(1,1,1)]
void computeMain()
{
    myNestedCall();
}

will output the following if compiled to glsl (note: the rework is to allow more complex cases to work as intended):

tests/language-feature/capability/capabilitySimplification3.slang(32): error 36107: entrypoint 'computeMain' does not support compilation target 'glsl' with stage 'compute'.
tests/language-feature/capability/capabilitySimplification3.slang(22): note: see using of 'WaveMultiPrefixProduct'
    WaveMultiPrefixProduct(1, 0);
    ^~~~~~~~~~~~~~~~~~~~~~
hlsl.meta.slang(15101): note: see definition of 'WaveMultiPrefixProduct'

jkwak-work · 2024-07-19T16:49:32Z

Sorry. I clicked a wrong button and accidently closed it; but reopened it right away.

jkwak-work · 2024-07-19T16:51:22Z

tests/language-feature/capability/capabilitySimplification3.slang(32): error 36107: entrypoint 'computeMain' does not support compilation target 'glsl' with stage 'compute'.
tests/language-feature/capability/capabilitySimplification3.slang(22): note: see using of 'WaveMultiPrefixProduct'
    WaveMultiPrefixProduct(1, 0);
    ^~~~~~~~~~~~~~~~~~~~~~
hlsl.meta.slang(15101): note: see definition of 'WaveMultiPrefixProduct'

This error message looks great.
But the "note" should also show one line above where "require" is described.

1. cheaper collection 2. don't give assumptions of problems, find the actual decl with incorrect capabilities. 3. change warning/error format to be more readable/useful

…/github.com/ArielG-NV/slang into implied-atom-removal-from-diagnostic-prints

ArielG-NV added the pr: non-breaking PRs without breaking changes label Jul 18, 2024

Merge branch 'master' into implied-atom-removal-from-diagnostic-prints

234a32d

ArielG-NV marked this pull request as draft July 18, 2024 18:51

ArielG-NV added 2 commits July 18, 2024 16:50

compress capability sets to remove redundant shader-stages for when p…

b5d0688

…rinting

Merge branch 'implied-atom-removal-from-diagnostic-prints' of https:/…

50b9aae

…/github.com/ArielG-NV/slang into implied-atom-removal-from-diagnostic-prints

ArielG-NV marked this pull request as ready for review July 18, 2024 20:51

ArielG-NV added 2 commits July 18, 2024 16:59

add a null case to printDiagnosticArg to ensure nothing outputs odd…

e7d9679

… text

test fix

ab213e9

jkwak-work reviewed Jul 19, 2024

View reviewed changes

csyonghe previously approved these changes Jul 19, 2024

View reviewed changes

csyonghe reviewed Jul 19, 2024

View reviewed changes

jkwak-work closed this Jul 19, 2024

jkwak-work reopened this Jul 19, 2024

ArielG-NV marked this pull request as draft July 19, 2024 18:24

change capability provenance collection and use

c52cdc4

1. cheaper collection 2. don't give assumptions of problems, find the actual decl with incorrect capabilities. 3. change warning/error format to be more readable/useful

ArielG-NV dismissed csyonghe’s stale review via c52cdc4 July 19, 2024 19:19

ArielG-NV marked this pull request as ready for review July 19, 2024 19:19

ArielG-NV added 5 commits July 19, 2024 15:19

Merge branch 'master' into implied-atom-removal-from-diagnostic-prints

6b381e3

push changes to add clarity to err

f2d0e1f

Merge branch 'implied-atom-removal-from-diagnostic-prints' of https:/…

5f5fa3d

…/github.com/ArielG-NV/slang into implied-atom-removal-from-diagnostic-prints

fix test error

d1d74f0

always print require if found since implies the result capabilities

07a2bbe

csyonghe approved these changes Jul 23, 2024

View reviewed changes

Merge branch 'master' into implied-atom-removal-from-diagnostic-prints

c2fabf6

ArielG-NV merged commit 509bfd8 into shader-slang:master Jul 23, 2024
11 checks passed

jkwak-work mentioned this pull request Jul 30, 2024

Slang build/release per sprint. #4718

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify `CapabilitySet` Diagnostic Printing #4678

Simplify `CapabilitySet` Diagnostic Printing #4678

ArielG-NV commented Jul 18, 2024 •

edited

Loading

csyonghe commented Jul 18, 2024

ArielG-NV commented Jul 18, 2024 •

edited

Loading

jkwak-work left a comment •

edited

Loading

csyonghe Jul 19, 2024

csyonghe Jul 19, 2024

ArielG-NV commented Jul 19, 2024 •

edited

Loading

jkwak-work commented Jul 19, 2024

jkwak-work commented Jul 19, 2024

Simplify CapabilitySet Diagnostic Printing #4678

Simplify CapabilitySet Diagnostic Printing #4678

Conversation

ArielG-NV commented Jul 18, 2024 • edited Loading

csyonghe commented Jul 18, 2024

ArielG-NV commented Jul 18, 2024 • edited Loading

jkwak-work left a comment • edited Loading

Choose a reason for hiding this comment

csyonghe Jul 19, 2024

Choose a reason for hiding this comment

csyonghe Jul 19, 2024

Choose a reason for hiding this comment

ArielG-NV commented Jul 19, 2024 • edited Loading

jkwak-work commented Jul 19, 2024

jkwak-work commented Jul 19, 2024

Simplify `CapabilitySet` Diagnostic Printing #4678

Simplify `CapabilitySet` Diagnostic Printing #4678

ArielG-NV commented Jul 18, 2024 •

edited

Loading

ArielG-NV commented Jul 18, 2024 •

edited

Loading

jkwak-work left a comment •

edited

Loading

ArielG-NV commented Jul 19, 2024 •

edited

Loading