-
Notifications
You must be signed in to change notification settings - Fork 706
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
[SPIRV] Emit DebugFunction/Definition for both wrapper and real functions #6966
[SPIRV] Emit DebugFunction/Definition for both wrapper and real functions #6966
Conversation
Lots of tests are failing. Try running the spir-v tests: |
3abc543
to
565af65
Compare
I'm sorry, I have one test left to fix. |
7cc8280
to
e3c8f76
Compare
@s-perron, the current failure in CI seems to be coming from an optimizer pass. I've attached the spvasm if we use fcgl or O0, and we see there are now 2 DebugFunctions, one for the wrapper and one for src.main. debug_info_manager.cpp:117 in the optimizer asserts. |
@SteveUrquhart The
|
6b5653a
to
b1789c8
Compare
@s-perron, I apologize for stepping in it again and not seeing that FunctionId duplication. Regarding the DebugFunctionDeclaration, the test we were discussing is using the OpenCL debug info instead of the NonSemantic debug info. The signature for DebugFunction is different between the two types, and I was messing up the forward reference in the OpenCL info. We don't emit the optional DebugFunctionDeclaration for now, for either debug info type. |
b1789c8
to
00f3c29
Compare
Apologies again for the delay in getting to this point. |
I've found the NonSemantic DebugScope for the entry function is incorrectly referencing the DebugFunction for the wrapper. I will enhance the testing to catch this and push again. |
00f3c29
to
3f1ea42
Compare
✅ With the latest revision this PR passed the C/C++ code formatter. |
3f1ea42
to
a0ebfbb
Compare
I've added testing for NonSemantic DebugScope, and cleaned up the names the debug user sees for the wrapper and the original function. While debugging src_main, the user will want to see "main" from the HLSL. |
I've found one more problem with the logic. The new convenience function emitDebugFunction() was the wrong place to pushCurrentLexicalScope(). I have a fix for this and am adding test logic for it now. |
I gonna check PR when you are ready. |
a0ebfbb
to
e1a4719
Compare
@Goshido, please check it when you are ready. |
#6758 unfortunately caused a regression for shaders compiled without -fcgl. This PR extends the original fix and corrects the regression. We now emit DebugFunction and DebugFunctionDefinition pairs for both the wrapper and the main functions, and a DebugEntrypoint that points at the wrapper function. This survives inlining.