Strip struct, entry and groupshared names from DXIL #7868
Conversation
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
@microsoft-github-policy-service agree |
Co-authored-by: Chris B <[email protected]>
Co-authored-by: Chris B <[email protected]>
Co-authored-by: Chris B <[email protected]>
|
@llvm-beanz Updated based on the review. Let me know if this is what you meant by using a separate pass. If not, please provide some guidance as to where and how it should work. |
GlassBeaver
force-pushed
the
strip_var_names
branch
from
efcc0d6 to
c57e18c
Compare
llvm-beanz
left a comment
llvm-beanz
left a comment
There was a problem hiding this comment.
I'll try to find some time this week to pull this down and refine the test cases. We should be able to write IR tests and other lit shell tests rather than TAEF tests to check all of this functionality.
|
Refactored the pass to roughly where advised and the functionality seems to still work and my tests pass, however CompilerTest::CompileThenTestPdbUtils is now failing. Since you've mentioned wanting to take a look at the tests later this week, I've left it in a failing state since I'm not sure what the interplay is between the passes and those tests with regards to data stripping, i.e. it could be that the failing test needs updating but also that my changes are wrong. |
|
Synced up to main. @llvm-beanz let me know if you need anything from me regarding those tests |
|
@llvm-beanz - have you had a chance to double check your feedback has been addressed? @GlassBeaver - could you add something to ReleaseNotes.md documenting this change please? |
|
|
||
| if (!GetShaderModel()->IsLib()) { | ||
| // Strip struct names | ||
| unsigned nextStructId = 0; |
There was a problem hiding this comment.
| unsigned nextStructId = 0; | |
| unsigned NextStructId = 0; |
nit: https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly.
There are some others lingering as well that would be nice to clean up.
| VERIFY_IS_NULL(hlsl::GetDxilPartByType(&header, hlsl::DxilFourCC::DFCC_DXIL)); | ||
| } | ||
|
|
||
| TEST_F(DxilContainerTest, StripReflectionRemovesStructNames) { |
There was a problem hiding this comment.
Regarding testing, ideally we could use the LIT/Filecheck infrastructure for directly comparing the modified LLVM IR before and after the new pass. The tools/clang/test/DXC/Passes/ has examples of this.
For instance, we would have an llvm ir module and have the run line be something like: %dxopt %s -hlsl-dxil-strip-debug-info -S | FileCheck %s.
It would demonstrate the names being present before the pass and then use CHECK directives to make sure they are removed after.
Similar to here.
4 participants
Strip struct, entry and groupshared names from DXIL when stripping reflection data.
#7627