dxcopt: Support full container and restore extra data to module #4845

tex3d · 2022-12-03T02:07:28Z

This modifies IDxcOptimizer::RunOptimizier to accept full DxilContainer input. When full container input is used, this restores some data that is stripped from the module and placed in various other container parts.

Data restored:

Subobjects from RDAT
RootSignature from RTS0
ViewID and I/O dependency data from PSV0
Resource names and types/annotations from STAT

Serialization of these to metadata in module bitcode output still requires hlsl-dxilemit step.

Fixes #4874

This modifies IDxcOptimizer::RunOptimizier to accept full DxilContainer input. When full container input is used, this also restores subobjects from RDAT or root signature data from RST0 to the DxilModule state. Serialization of these to metadata in module bitcode output still requires hlsl-dxilemit step.

lib/HLSL/DxcOptimizer.cpp

AppVeyorBot · 2022-12-03T03:04:23Z

❌ Build DirectXShaderCompiler 1.0.2349 failed (commit 38a54aad92 by @tex3d)

AppVeyorBot · 2022-12-03T04:34:28Z

❌ Build DirectXShaderCompiler 1.0.2351 failed (commit 820b4beff2 by @tex3d)

AppVeyorBot · 2022-12-03T05:41:27Z

❌ Build DirectXShaderCompiler 1.0.2352 failed (commit c1695ea9f7 by @tex3d)

AppVeyorBot · 2022-12-03T06:07:32Z

❌ Build DirectXShaderCompiler 1.0.2353 failed (commit 1a3253b270 by @tex3d)

AppVeyorBot · 2022-12-03T20:46:18Z

❌ Build DirectXShaderCompiler 1.0.2355 failed (commit c66f216376 by @jeffnn)

AppVeyorBot · 2022-12-04T00:17:27Z

❌ Build DirectXShaderCompiler 1.0.2356 failed (commit acf132ae91 by @jeffnn)

AppVeyorBot · 2022-12-06T03:49:32Z

✅ Build DirectXShaderCompiler 1.0.2366 completed (commit facc8071f2 by @jeffnn)

jeffnn

Confirmed that this fixes the PIX scenario.

AppVeyorBot · 2022-12-07T19:25:19Z

✅ Build DirectXShaderCompiler 1.0.2384 completed (commit ac8d443bef by @jeffnn)

lib/HLSL/DxcOptimizer.cpp

tex3d · 2022-12-07T19:50:12Z

tools/clang/unittests/HLSL/OptimizerTest.cpp

+  VERIFY_SUCCEEDED(m_dllSupport.CreateInstance(CLSID_DxcContainerReflection, &pReflection));
+  VERIFY_SUCCEEDED(pReflection->Load(pOptimizedContainer));
+  UINT32 dxilIndex;
+  VERIFY_SUCCEEDED(pReflection->FindFirstPartKind(hlsl::DFCC_DXIL, &dxilIndex));


Is there a reason this uses container reflection here, then the lower-level DxilContainer code below? Either one works, but it doesn't seem necessary to use a different method when getting a different part.

Just because I had lifted the lower-level code from elsewhere prior to deleting the code in the optimizer that serialized the dxil to container.

tex3d · 2022-12-07T19:52:49Z

tools/clang/unittests/HLSL/OptimizerTest.cpp

+      reinterpret_cast<const char *>(pOptimizedContainer->GetBufferPointer());
+  unsigned blobSize = pOptimizedContainer->GetBufferSize();
+  const hlsl::DxilContainerHeader *pContainerHeader =
+      hlsl::IsDxilContainerLike(pBlobContent, blobSize);


Just a note: After IsDxilContainerLike, you can also use IsValidDxilContainer to make sure the container is well-formed (though it's unlikely to be badly formed on output from AssembleToContainer).

Small refactor to separate code that translates between ViewID data format serialized in DxilModule and the PSV0 data format. Added the equivalent functions to go back to DxilModule serialized version from PSV0 used in IDxcOptimizer.

tex3d · 2022-12-08T00:11:03Z

tools/clang/unittests/HLSL/OptimizerTest.cpp

+    hlsl::DXIL::SubobjectKind subobjectKind = subObject.getKind();
+    switch (subobjectKind) {
+    case hlsl::DXIL::SubobjectKind::StateObjectConfig:
+      VERIFY_IS_TRUE(0 == strcmp(subObject.getName(), "so_StateObjectConfig"));


I think it's better to use VERIFY_ARE_EQUAL_STR, since that will be more informative if it fails.

AppVeyorBot · 2022-12-08T00:53:10Z

✅ Build DirectXShaderCompiler 1.0.2387 completed (commit f6e6a4cd00 by @tex3d)

Also, make sources const.

adam-yang · 2022-12-09T22:53:21Z

lib/DxilContainer/DxilContainerAssembler.cpp

+// ComputeViewIdState. It could be moved into some common location if this
+// ViewID serialization/deserialization code were moved out of here.
+static unsigned RoundUpToUINT(unsigned x) { return (x + 31) / 32; }
+static unsigned ComputeSeriaizedViewIDStateSizeInUInts(


Are we going to make ComputeViewIdState call this?

Ideally, this could live in a common location, but I haven't yet figured out where that should be. Hence the comment on the function.

I do plan to clean this up along with the format inconsistency between module and PSV0 in the near future, so I'd rather clean this up at that point.

tex3d · 2022-12-09T23:43:14Z

tools/clang/unittests/HLSL/OptimizerTest.cpp

-    VERIFY_ARE_EQUAL(originalRes->GetResDimName(), optimizedRes->GetResDimName());
-    VERIFY_ARE_EQUAL(originalRes->GetResIDPrefix(), optimizedRes->GetResIDPrefix());
+    VERIFY_ARE_EQUAL_STR(originalRes->GetResBindPrefix(), optimizedRes->GetResBindPrefix());
+    VERIFY_ARE_EQUAL_STR(originalRes->GetResIDPrefix(), optimizedRes->GetResIDPrefix());


These are just the prefix character decoded from the Class, so comparing GetResBindPrefix and GetResIDPrefix are also unnecessary. Basically, if Class was wrong, there would be something very fundamentally wrong with which list the resource is in for the DxilModule. By which I mean we would have much bigger problems, and expect a whole lot of things to break.

AppVeyorBot · 2022-12-10T02:35:53Z

❌ Build DirectXShaderCompiler 1.0.2418 failed (commit cf55aba051 by @tex3d)

AppVeyorBot · 2022-12-10T03:35:09Z

✅ Build DirectXShaderCompiler 1.0.2419 completed (commit d03b146981 by @tex3d)

AppVeyorBot · 2022-12-10T05:37:11Z

✅ Build DirectXShaderCompiler 1.0.2421 completed (commit 1d3323d150 by @jeffnn)

AppVeyorBot · 2022-12-10T06:38:06Z

✅ Build DirectXShaderCompiler 1.0.2422 completed (commit 2abb9421de by @jeffnn)

AppVeyorBot · 2022-12-10T09:39:41Z

✅ Build DirectXShaderCompiler 1.0.2425 completed (commit 6c865ae7c6 by @jeffnn)

AppVeyorBot · 2022-12-10T10:42:34Z

✅ Build DirectXShaderCompiler 1.0.2426 completed (commit ccab925f1a by @jeffnn)

AppVeyorBot · 2022-12-10T11:38:52Z

✅ Build DirectXShaderCompiler 1.0.2427 completed (commit 07e18ea352 by @jeffnn)

AppVeyorBot · 2022-12-10T14:43:10Z

✅ Build DirectXShaderCompiler 1.0.2431 completed (commit 2258c9284f by @jeffnn)

AppVeyorBot · 2022-12-10T16:49:12Z

✅ Build DirectXShaderCompiler 1.0.2433 completed (commit e5b460438d by @jeffnn)

adam-yang

LGTM!

PIX is unique in that it needs to deserialize, modify and then reserialize root sigs. The focus of this checkin is adding such modifications to PixPassHelpers.cpp. (This closes a gap in PIX support: PIX can now support shaders that use dxil-defined and attribute-style root signatures.) But this required some work outside of the purely PIX-focused areas. Deserialized root sigs are described by a C-like structure with embedded arrays that are new/delete-ed by routines in DxilRootSignature.cpp. Since modifying these structs requires more new/delete calls, I chose to add a new entry point in DxilRootSignature.cpp to do the bare minimum that PIX needs: extend root params by one descriptor. This approach keeps all those raw new/deletes in one file. I found a leak in DxilRootSignatureSerialzier.cpp, which I fixed. There appear to be no unit tests that exercise this path. I welcome feedback on adding one. There were other leaks in class CVersionedRootSignatureDeserializer, but this class is unused so I deleted it. Oh, and there are bazillions of commits because I was cherry-picking from a recent change (#4845) as it eveolved, since I needed that change and this to test PIX.

…soft#4876) PIX is unique in that it needs to deserialize, modify and then reserialize root sigs. The focus of this checkin is adding such modifications to PixPassHelpers.cpp. (This closes a gap in PIX support: PIX can now support shaders that use dxil-defined and attribute-style root signatures.) But this required some work outside of the purely PIX-focused areas. Deserialized root sigs are described by a C-like structure with embedded arrays that are new/delete-ed by routines in DxilRootSignature.cpp. Since modifying these structs requires more new/delete calls, I chose to add a new entry point in DxilRootSignature.cpp to do the bare minimum that PIX needs: extend root params by one descriptor. This approach keeps all those raw new/deletes in one file. I found a leak in DxilRootSignatureSerialzier.cpp, which I fixed. There appear to be no unit tests that exercise this path. I welcome feedback on adding one. There were other leaks in class CVersionedRootSignatureDeserializer, but this class is unused so I deleted it. Oh, and there are bazillions of commits because I was cherry-picking from a recent change (microsoft#4845) as it eveolved, since I needed that change and this to test PIX. (cherry picked from commit 20bb3d0)

…osoft#4845) This modifies IDxcOptimizer::RunOptimizier to accept full DxilContainer input. When full container input is used, this restores some data that is stripped from the module and placed in various other container parts. Data restored: - Subobjects from RDAT - RootSignature from RTS0 - ViewID and I/O dependency data from PSV0 - Resource names and types/annotations from STAT Serialization of these to metadata in module bitcode output still requires hlsl-dxilemit step. (cherry picked from commit 2c3d965)

Cherry-pick of PIX and HLK changes for the 2212 release Changes for PIX: 20bb3d0 PIX: Modify root sigs in place (plus fix root sig memory leak) (PIX: Modify root sigs in place (plus fix root sig memory leak) #4876) 2c3d965 dxcopt: Support full container and restore extra data to module (dxcopt: Support full container and restore extra data to module #4845) 21cf36a Fix hitgroup metadata argument order HLK Test Updates: ee0994e add barycentrics ordering check onto existing barycentrics test (add barycentrics ordering check onto existing barycentrics test #4635) 6acd11b ConvertFloat32ToFloat16: Use DirectXMath conversion functions (ConvertFloat32ToFloat16: Use DirectXMath conversion functions #4855) e7aac8e Include TestConfig.h only if DEFAULT_TEST_DIR is not defined (Include TestConfig.h only if DEFAULT_TEST_DIR is not already defined #4884) 5decc4a Do not include TestConfig.h for all HLK build (Do not include TestConfig.h for all HLK build #4887)

adam-yang reviewed Dec 3, 2022

View reviewed changes

lib/HLSL/DxcOptimizer.cpp Show resolved Hide resolved

Move more code into the try block.

9956bac

tex3d added 2 commits December 2, 2022 19:09

Beginning outline of tests

dea301d

Better return code when container doesn't contain DXIL

dd5b6bd

Assemble to container, add a test

f91f943

More test

5f72f78

Fix ubuntu build?

7c13794

Assemble to container outside optimizer

b91c86f

jeffnn marked this pull request as ready for review December 7, 2022 19:15

jeffnn approved these changes Dec 7, 2022

View reviewed changes

tex3d commented Dec 7, 2022

View reviewed changes

lib/HLSL/DxcOptimizer.cpp Outdated Show resolved Hide resolved

tex3d commented Dec 7, 2022

View reviewed changes

tex3d commented Dec 8, 2022

View reviewed changes

tex3d and others added 5 commits December 7, 2022 16:59

Restore resource names from shader reflection part (STAT)

99abb03

Restore Resource HLSL types and type annotations

ff66248

Also, make sources const.

Some error checking when copying resource info

ecc4330

Remove existing type annotation if present, to force replacement

2a4e96f

Start of view id tests

e2adc94

Test fixes

a3733c9

adam-yang reviewed Dec 9, 2022

View reviewed changes

jeffnn added 2 commits December 9, 2022 14:54

test fixes

89ddbb7

Undo mistaken checkin

e823397

tex3d commented Dec 9, 2022

View reviewed changes

jeffnn added 2 commits December 9, 2022 17:34

Tests for i/o dependency map

190f138

Test all tables

0182587

adam-yang approved these changes Dec 13, 2022

View reviewed changes

tex3d changed the title ~~dxcopt: Support full container and restore subobjects/root sig to module~~ dxcopt: Support full container and restore extra data to module Dec 13, 2022

NC: Update comment for what's restored from STAT

1cfd5e7

tex3d enabled auto-merge (squash) December 13, 2022 01:23

tex3d merged commit 2c3d965 into microsoft:main Dec 13, 2022

jeffnn mentioned this pull request Dec 13, 2022

PIX: Modify root sigs in place (plus fix root sig memory leak) #4876

Merged

tex3d deleted the dxcopt-from-container branch December 13, 2022 20:35

hekota mentioned this pull request Dec 16, 2022

Cherry-pick of PIX and HLK changes for the 2212 release #4891

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dxcopt: Support full container and restore extra data to module #4845

dxcopt: Support full container and restore extra data to module #4845

tex3d commented Dec 3, 2022 •

edited

Loading

AppVeyorBot commented Dec 3, 2022

AppVeyorBot commented Dec 3, 2022

AppVeyorBot commented Dec 3, 2022

AppVeyorBot commented Dec 3, 2022

AppVeyorBot commented Dec 3, 2022

AppVeyorBot commented Dec 4, 2022

AppVeyorBot commented Dec 6, 2022

jeffnn left a comment

AppVeyorBot commented Dec 7, 2022

tex3d Dec 7, 2022

jeffnn Dec 7, 2022

tex3d Dec 7, 2022

tex3d Dec 8, 2022

AppVeyorBot commented Dec 8, 2022

adam-yang Dec 9, 2022

tex3d Dec 9, 2022

tex3d Dec 13, 2022

tex3d Dec 9, 2022

AppVeyorBot commented Dec 10, 2022

AppVeyorBot commented Dec 10, 2022

AppVeyorBot commented Dec 10, 2022

AppVeyorBot commented Dec 10, 2022

AppVeyorBot commented Dec 10, 2022

AppVeyorBot commented Dec 10, 2022

AppVeyorBot commented Dec 10, 2022

AppVeyorBot commented Dec 10, 2022

AppVeyorBot commented Dec 10, 2022

adam-yang left a comment

dxcopt: Support full container and restore extra data to module #4845

dxcopt: Support full container and restore extra data to module #4845

Conversation

tex3d commented Dec 3, 2022 • edited Loading

AppVeyorBot commented Dec 3, 2022

AppVeyorBot commented Dec 3, 2022

AppVeyorBot commented Dec 3, 2022

AppVeyorBot commented Dec 3, 2022

AppVeyorBot commented Dec 3, 2022

AppVeyorBot commented Dec 4, 2022

AppVeyorBot commented Dec 6, 2022

jeffnn left a comment

Choose a reason for hiding this comment

AppVeyorBot commented Dec 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AppVeyorBot commented Dec 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AppVeyorBot commented Dec 10, 2022

AppVeyorBot commented Dec 10, 2022

AppVeyorBot commented Dec 10, 2022

AppVeyorBot commented Dec 10, 2022

AppVeyorBot commented Dec 10, 2022

AppVeyorBot commented Dec 10, 2022

AppVeyorBot commented Dec 10, 2022

AppVeyorBot commented Dec 10, 2022

AppVeyorBot commented Dec 10, 2022

adam-yang left a comment

Choose a reason for hiding this comment

tex3d commented Dec 3, 2022 •

edited

Loading