Skip to content
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

Assert on static const in template functions #5273

Closed
MarijnS95 opened this issue Jun 6, 2023 · 1 comment · Fixed by #5382
Closed

Assert on static const in template functions #5273

MarijnS95 opened this issue Jun 6, 2023 · 1 comment · Fixed by #5382
Assignees
Labels
bug Bug, regression, crash hlsl2021 Pertaining to HLSL2021 features spirv Work related to SPIR-V

Comments

@MarijnS95
Copy link
Contributor

MarijnS95 commented Jun 6, 2023

Compiling some HLSL code with static const in templates seems to be fine in the release builds, but when asserts are turned on in a debug build the following appears:

DirectXShaderCompiler/tools/clang/lib/SPIRV/DeclResultIdMapper.cpp:4104: clang::QualType clang::spirv::DeclResultIdMapper::getTypeAndCreateCounterForPotentialAliasVar(const clang::DeclaratorDecl *, bool *): Assertion `!varDecl->isExternallyVisible() || varDecl->isStaticDataMember()' failed.

Repro with:

template <typename R> R test(R x) {
    static const R v = 20;
    return v * x;
}

[numthreads(32, 32, 1)] void main(uint2 threadId: SV_DispatchThreadID) {
    float x = test(10);
}

Should this be turned into an emitError() to disallow the case globally, with a fitting error message? I haven't checked if the code generates and runs as expected, but it seems so.

@llvm-beanz llvm-beanz added the needs-triage Awaiting triage label Jun 29, 2023
@llvm-beanz llvm-beanz added spirv Work related to SPIR-V and removed needs-triage Awaiting triage labels Jun 30, 2023
@llvm-beanz llvm-beanz moved this to For Google in HLSL Triage Jun 30, 2023
@llvm-beanz llvm-beanz added needs-triage Awaiting triage hlsl2021 Pertaining to HLSL2021 features labels Jun 30, 2023
@s-perron s-perron added the bug Bug, regression, crash label Jun 30, 2023
@s-perron
Copy link
Collaborator

This test case should pass. The code that is generated when the assert is disabled is correct. We just need to fix the assert. It seems too restrictive.

@s-perron s-perron moved this from For Google to Done in HLSL Triage Jun 30, 2023
@s-perron s-perron removed the needs-triage Awaiting triage label Jun 30, 2023
s-perron added a commit to s-perron/DirectXShaderCompiler that referenced this issue Jun 30, 2023
There is an assert that checks if a variable will be placed in the
Function or Private storage class in the spir-v. However, the condition
checked in that assert is inconsistent with the part of the code that
determines which storage class a variable belongs to.

The fix is to make the function `isExternalVar` available outside of
SprivEmitter.cpp, and use that in the assert. Now the same check is used
everywhere.

Fixes microsoft#5273
@s-perron s-perron self-assigned this Jun 30, 2023
@pow2clk pow2clk moved this to 🏗 In progress in HLSL Roadmap Jul 6, 2023
@pow2clk pow2clk moved this from 🏗 In progress to 👀 In review in HLSL Roadmap Jul 6, 2023
@pow2clk pow2clk removed this from HLSL Triage Jul 6, 2023
s-perron added a commit that referenced this issue Jul 7, 2023
There is an assert that checks if a variable will be placed in the
Function or Private storage class in the spir-v. However, the condition
checked in that assert is inconsistent with the part of the code that
determines which storage class a variable belongs to.

The fix is to make the function `isExternalVar` available outside of
SprivEmitter.cpp, and use that in the assert. Now the same check is used
everywhere.

Fixes #5273
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in HLSL Roadmap Jul 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Bug, regression, crash hlsl2021 Pertaining to HLSL2021 features spirv Work related to SPIR-V
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants