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

[SPIRV] Emit DebugFunction/Definition for both wrapper and real functions #6966

Merged
merged 1 commit into from
Nov 19, 2024

Conversation

SteveUrquhart
Copy link
Contributor

#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.

@s-perron
Copy link
Collaborator

Lots of tests are failing. Try running the spir-v tests: ninja check-clang-codegenspirv.

@SteveUrquhart
Copy link
Contributor Author

I'm sorry, I have one test left to fix.

@SteveUrquhart SteveUrquhart force-pushed the fix-ns-debugfunctiondef branch 2 times, most recently from 7cc8280 to e3c8f76 Compare October 17, 2024 19:55
@SteveUrquhart
Copy link
Contributor Author

@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.
assert(
fn_id_to_dbg_fn_.find(fn_id) == fn_id_to_dbg_fn_.end() &&
"Register DebugFunction for a function that already has DebugFunction");
I appreciate your advice on this.
spirv.debug.opline.end.of.shader.O0.spvasm.txt
spirv.debug.opline.end.of.shader.fcgl.spvasm.txt

@s-perron
Copy link
Collaborator

@SteveUrquhart The DebugFunction instructions do not look correct. Both of them have %src_main for their declaration, which is incorrect for two reasons. First they represent different functions, so they should be different. Second, that value should be the result of a DebugFunctionDeclaration, and not the result of the OpFunction.

   %src_main = OpFunction %void None %15
         %10 = OpExtInst %void %1 DebugFunction %8 %5 %6 3 1 %7 %9 FlagIsProtected|FlagIsPrivate 3 %src_main
         %14 = OpExtInst %void %1 DebugFunction %13 %5 %6 3 1 %7 %9 FlagIsProtected|FlagIsPrivate 3 %src_main

@SteveUrquhart SteveUrquhart force-pushed the fix-ns-debugfunctiondef branch 2 times, most recently from 6b5653a to b1789c8 Compare October 21, 2024 17:55
@SteveUrquhart
Copy link
Contributor Author

@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.
I've pushed what I think is correct for both debug info types. There are still a handful of failures in the rich.debig.info tests, but I believe these to all be false failures due to rearranging of debug info. I don't want to be hasty again so I'm looking through those now.

@SteveUrquhart
Copy link
Contributor Author

Apologies again for the delay in getting to this point.

@SteveUrquhart
Copy link
Contributor Author

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.

Copy link
Contributor

github-actions bot commented Oct 23, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

@SteveUrquhart
Copy link
Contributor Author

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.

@s-perron
Copy link
Collaborator

This looks good to me, but I'm not setup to test it in renderdoc. @Goshido Does this fix #6939?

@SteveUrquhart
Copy link
Contributor Author

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.

@Goshido
Copy link

Goshido commented Oct 28, 2024

I gonna check PR when you are ready.

@SteveUrquhart
Copy link
Contributor Author

@Goshido, please check it when you are ready.

@Goshido
Copy link

Goshido commented Oct 29, 2024

I checked branch fix-ns-debugfunctiondef, commit e1a4719d7b53ef6fddffa3f18422b6526f65a79b. DXC version 1.8.2407.93. It's working as expected:
image

Awesome! 👍

P.S. Quick question. What last digits of DXC version mean?

            minor   ???
              |      |
              ▼      ▼
DXC version 1.8.2407.93
            ^     ^
            |     |
          major   |
                  |
             year and ???

@s-perron s-perron enabled auto-merge (squash) November 18, 2024 16:35
@s-perron s-perron merged commit 891392e into microsoft:main Nov 19, 2024
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants