Capability System: Implicit capability upgrade warning/error #4241
Conversation
adjusted implementation + tests to support a warning/error if capabilities are implicitly upgraded and test accordingly.
ArielG-NV
changed the title
|
note: profile is automatically set in render test sometimes, likely need to fix capability representation in .capdef+profile (#4163) to avoid unwanted errors. |
ArielG-NV
force-pushed
the
capabilities-warn-or-error-if-upgraded-implicitly
branch
from
aaea056 to
d92292d
Compare
1. changed testing infrastructure to not set a `profile` explicitly, 2. Added tests to be sure this works as intended with user API and with slangc command line
…errors 1. most `glsl_spirv` version atoms have been removed from `.capdef`, instead we will translate `spirv` version atoms into `glsl_spirv` since there is no point in writing the same code twice in `.capdef` files to define `spirv` versions. 2. add spirv version, and hlsl sm version (and equivlent) capability dependencies 3. removed some stage requirments which were set on objects, keep the wrapper capabilities. I am keeping the wrapper capabilities since I am unaware on if there are stage limitations (spec says code in practice does not work).
…bilities-warn-or-error-if-upgraded-implicitly
…ttps://github.com/ArielG-NV/slang into capabilities-warn-or-error-if-upgraded-implicitly
many mistakes were mesh shaders doing `-profile glsl_450+spirv_1_4`. This is not allowed for a few reasons 1. the test tooling does not handle arguments the same as `slangc` 2. glsl_450 core profile does not support mesh shaders, nor does spirv_1_4. sm_6_5 does work in this senario
this is required since sm version changes compiled code & behavior
There was a problem hiding this comment.
In all of our capability related diagnostic messages, we should try to simplify the atoms so that if a implies b, we don't print out b. For example, if we want to report spirv_1_5, we just report spirv_1_5 instead of spirv_1_0+spirv_1_1+spirv_1_2+spirv_1_3+spirv_1_4+spirv_1_5.
source/slang/slang-compiler.h
Outdated
| @@ -3324,6 +3330,34 @@ struct CompileTimerRAII | |||
| session->addTotalCompileTime(elapsedTime); | |||
| } | |||
| }; | |||
|
|
|||
| // helpers for error/warning reporting | |||
| enum class TypeOfErrorToDiagnose | |||
new issue or add to this issue? |
source/slang/slang-capability.h
Outdated
| enum class ImpliesReturnFlags : int | ||
| { | ||
| NotImplied = 0, | ||
| Implied = 1 << 1, |
|
@ArielG-NV Can do a new issue to simplify capability printing. |
| ; | ||
| alias spirv_latest = _spirv_1_6; | ||
|
|
||
| alias sm_4_0_version = _sm_4_0 |
There was a problem hiding this comment.
Maybe add a comment here on what x_version means, it means x for the corresponding target, and the closest match of equivvalent feature set for other targets.
There was a problem hiding this comment.
do we want to automatically translate spirv_1_0 to spirv_1_0_version when the user specifies slangc shader.slang -profile spirv_1_0_version -target hlsl?
There was a problem hiding this comment.
do we want to automatically translate spirv_1_0 to spirv_1_0_version when the user specifies slangc shader.slang -profile spirv_1_0_version -target hlsl?
-
I did not add spirv_*_version but that is a good idea.
-
We probably also should also warn if a user uses any capability defined as internal in
slangc(_*)
Should '1'/'2' be another issue or part of this PR?
source/slang/slang-capability.cpp
Outdated
| return CapabilityName::Invalid; | ||
| } | ||
|
|
||
| void CapabilitySet::AddSpirvVersionFromOtherAsGlslSpirvVersion(CapabilitySet& other) |
There was a problem hiding this comment.
naming conversion. Add->add.
…ttps://github.com/ArielG-NV/slang into capabilities-warn-or-error-if-upgraded-implicitly
…bilities-warn-or-error-if-upgraded-implicitly
|
It seems this is breaking the |
I will check it out |
|
I still need to figure out why 64 bit works while 32bit fails to draw conclusions |
|
+1 on the simplified compatibility printing. The compatibility system is a good idea, but with too complex of an error printed, I’m more likely to disable the feature than to address it. Programmers are very susceptible to alarm fatigue. |
|
#4347 is tracking the simplified compatibility printing issue |
v2024.1.22 c00f461 remove inline that crashes old glibc version (shader-slang#4398) fdef653 Improve Direct SPIRV Backend Test Coverage (shader-slang#4396) 33e81a0 [Metal] Fix global constant array emit. (shader-slang#4392) a38a4fb Make unknown attributes a warning instead of an error. (shader-slang#4391) a210091 [Metal] Support SV_TargetN. (shader-slang#4390) 2cc9690 Implement for metal `SV_GroupIndex` (shader-slang#4385) a6b8348 fix the clang/gcc warning (shader-slang#4382) cfef0c6 Metal: misc fixes and enable more tests. (shader-slang#4374) 2407966 SPIR-V `SV_InstanceId` support in pixel shader (shader-slang#4368) fba316f Remove `IRHLSLExportDecoration` and `IRKeepAliveDecoration` for non-CUDA/Torch targets (shader-slang#4364) f0d40ad (origin/master, origin/HEAD, master) capture/replay: implement infrastructure for capture (shader-slang#4372) ecc6ecb Fix cuda/cpp/metal crash for when using GLSL style shader inputs (shader-slang#4378) 0bf0bf7 Implement Sampler2D for CPP target (shader-slang#4371) b970b88 Enable full test on macos. (shader-slang#4327) 0574dca Delete glsl_vulkan and glsl_vulkan_one_desc targets. (shader-slang#4361) 085d1a6 Fix emit logic for getElementPtr. (shader-slang#4362) 8813c61 Capability System: Implicit capability upgrade warning/error (shader-slang#4241) 7447fca Add constant folding for % operator. (shader-slang#4359) 8bf7d11 Fix merge error. (shader-slang#4358) b7e8243 Add slangc flag to `-zero-initialize` all variables (shader-slang#3987) ccc26c2 Extend the COM-based API to support whole program compilation. (shader-slang#4355) 318adcc Add compiler option to treat enum types as unscoped. (shader-slang#4354) ec35feb Fix incorrect drop of decoration when translating glsl global var to entrypoint param. (shader-slang#4353) c194af8 Fix crash on invalid entrypoint varying parameter. (shader-slang#4349) 7a4757d Implicit register binding for hlsl to non-hlsl targets (shader-slang#4338) 180d6b1 Fix duplicate SPIRV decorations. (shader-slang#4346) fa8c11e Add option to preserve shader parameter declaration in output SPIRV. (shader-slang#4344) 3fe4a77 Fix crash when using optional type in a generic. (shader-slang#4341) 5da06d4 Fix global value inlining for spirv_asm blocks. (shader-slang#4339) 7e79669 [gfx] Metal improvements (shader-slang#4337) 6909d65 SPIRV backend: add support for tessellation stages, (shader-slang#4336) ef20d93 Test more texture types in Metal (shader-slang#4333) 5a28968 [gfx] Metal texture fixes (shader-slang#4331) df0a201 Support integer typed textures for GLSL (shader-slang#4329) 51d3585 Remove duplicate `VkPhysicalDeviceComputeShaderDerivativesFeaturesNV` extension structure in vk-api.h (shader-slang#4335) 6d5ef9b Fix `GetAttributeAtVertex` for spirv and glsl targets. (shader-slang#4334) 21bbebb Address glslang ordering requirments for 'derivative_group_*NV' (shader-slang#4323) 72016f9 Partial implementation of static_assert (shader-slang#4294) 712ce65 enable more metal tests (shader-slang#4326) 38c0bac Fix SPIRV emit for `Flat` decoration and TessLevel builtin. (shader-slang#4318) b5cdd83 Support all integer typed indices in StructuredBuffer Load/Store/[]. (shader-slang#4311) 6857dd5 [gfx] Metal graphics support (shader-slang#4324) 0974463 Fix typos in the docs (shader-slang#4322) 9a23a9a SPIRV `Block` decoration fixes. (shader-slang#4303) bc680e7 Add initial draft auto-diff basics and IR overview documents (shader-slang#4216) ee812d1 Disallow certain types of decls in `interface` to provide better diagnostic message. (shader-slang#4312) 65928af Metal system value overhaul. (shader-slang#4308) e39ceab Adding functional test for GLSL texture functions (shader-slang#4306) 2a45bc3 Support HLSL `.mips` syntax. (shader-slang#4310) 056a4b9 Small SPIRV emit cleanup around vector element extract. (shader-slang#4309) f83fe55 Make CTS failure report more obvious (shader-slang#4302) 78d34f3 Improve documentation and example formatting consistency (shader-slang#4299) 7c6faf6 Precompute UIntSet from individual capabilities inside generator (shader-slang#4269) 004fe27 Metal compute tests (shader-slang#4292) 72f10a8 Fixed profile string per request in pr#4268 (shader-slang#4297) a709e02 Update capability-generator-main.cpp (shader-slang#4295) d301267 Typo in 06-interfaces-generics.md (shader-slang#4284) fa664d1 Fix build warnings and treat warnings as error on CI (shader-slang#4276) f149052 Remove unnecessary call to __requireComputeDerivative (shader-slang#4283)
fixes: #4164
fixes: #4165
fixes: #4317
Support a warning/error if capabilities are implicitly upgraded and test accordingly.
Notable changes:
ImpliesReturnFlags&ImpliesFlagswas added to removeboolparameters for managingimpliesand to allow for a clearer "implies" operation usage pattern.maybeDiagnosereplacesdiagnoseCapabilityErrorsto allow for a more general "type" of error. This was done since "maybeDiagnoseWarningOrError" was added (for togglable warning<->error), and requires a diagnosis wrapper which may not diagnose an error for use with capabilities._spirv_*have been added to handle 'spirv version' capabilities,spirv_*are now being used as 'spirv core' (extensions promoted to core are associated withspirv_*)._spirv_*now translates intoglsl_spirv_*. This was done since there is not much of a reason to manually specifyglsl_spirv_*whenspirvtarget has a subset of the capabilities ofglsl_spirv_*(subset since glslang bugs sometimes require inflated versions).GLSL_*andsm_*specify a version and cross-platform representation of extensions of the version.