Execution Tests: Clean up - Factor out createWARPDevice and createHardwareDevice by alsepkow · Pull Request #7987 · microsoft/DirectXShaderCompiler

alsepkow · 2025-12-06T00:50:03Z

This PR just cleans up the createDevice function by factoring out the logic for creating a WARP device. And I also included a suggestion from co-pilot to add a small RAII wrapper for managing the lifetime of the Warp DLL.

github-actions · 2025-12-06T00:50:53Z

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff ba86cc068a61dc890c244bd8a710e704cebf327b b2531d6c2cacda27d13882e2e8ed9852d80676b5 -- tools/clang/unittests/HLSLExec/HlslExecTestUtils.cpp

View the diff from clang-format here.

diff --git a/tools/clang/unittests/HLSLExec/HlslExecTestUtils.cpp b/tools/clang/unittests/HLSLExec/HlslExecTestUtils.cpp
index 0af439a0..095524ff 100644
--- a/tools/clang/unittests/HLSLExec/HlslExecTestUtils.cpp
+++ b/tools/clang/unittests/HLSLExec/HlslExecTestUtils.cpp
@@ -83,7 +83,7 @@ static void createWarpDevice(
         CreateDeviceFn,
     ID3D12Device **D3DDevice, bool SkipUnsupported) {
 
-   if (*D3DDevice)
+  if (*D3DDevice)
     LogWarningFmt(L"createDevice called with non-null *D3DDevice - "
                   L"this will likely leak the previous device");
   *D3DDevice = nullptr;
@@ -152,7 +152,7 @@ static void createHardwareDevice(
         CreateDeviceFn,
     ID3D12Device **D3DDevice) {
 
-   if (*D3DDevice)
+  if (*D3DDevice)
     LogWarningFmt(L"createDevice called with non-null *D3DDevice - "
                   L"this will likely leak the previous device");
   *D3DDevice = nullptr;

Check this box to apply formatting changes to this branch.

damyanp · 2025-12-08T20:51:15Z

The previous version had comments explaining the strategy around loading the right DLL etc, it's shame these have all been lost in this refactor.

alsepkow · 2025-12-08T20:55:26Z

I'll add the comments back in.

…

________________________________ From: Damyan Pepper ***@***.***> Sent: Monday, December 8, 2025 12:51 PM To: microsoft/DirectXShaderCompiler ***@***.***> Cc: Alex Sepkowski ***@***.***>; Author ***@***.***> Subject: Re: [microsoft/DirectXShaderCompiler] Execution Tests: Clean up - Factor out createWARPDevice and add RAII wrapper for WARP dll. (PR #7987) [https://avatars.githubusercontent.com/u/8118402?s=20&v=4]damyanp left a comment (microsoft/DirectXShaderCompiler#7987)<#7987 (comment)> The previous version had comments explaining the strategy around loading the right DLL etc, it's shame these have all been lost in this refactor. — Reply to this email directly, view it on GitHub<#7987 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABK4EW7BWBVND3G6WDHHTBT4AXQFRAVCNFSM6AAAAACOGT2BFOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTMMRYHE2TGOBXGE>. You are receiving this because you authored the thread.Message ID: ***@***.***>

damyanp · 2025-12-08T20:59:30Z

tools/clang/unittests/HLSLExec/HlslExecTestUtils.cpp

+    createWarpDevice(DXGIFactory, CreateDeviceFn, &D3DDeviceCom,
+                     SkipUnsupported);
  } else {
    CComPtr<IDXGIAdapter1> HardwareAdapter;


Paraphrazing:

if (UseWrap)
callSingleFunctionThatDoesTheUseWarpThing();
else {
// many lines of code for doing the hardware device thing
}

See "single level of abstraction principle".

This change does muddy that a bit. But on the other hand, I felt that a 100 line function for creating a d3d device was getting overly complicated and felt difficult to follow.

Maybe it would be better to move the DLL loader back to a class. It can handle the logging/logic for that.
And we could have shared logic to call CreateDeviceFn. The branching logic based on SW/HW can just select the appropriate IDXGIAdapter before calling.

Or I can leave it all alone. This function has just been eating at me for a while.

My suggestion would have been to add a createHardwareDevice() that's at the same level of abstraction os createWarpDevice().

damyanp · 2025-12-08T21:02:48Z

tools/clang/unittests/HLSLExec/HlslExecTestUtils.cpp

+  // Log the actual version of WARP that's loaded
+  if (GetModuleHandleW(L"d3d10warp.dll") != NULL) {
+    WCHAR FullModuleFilePath[MAX_PATH] = L"";
+    GetModuleFileNameW(GetModuleHandleW(L"d3d10warp.dll"), FullModuleFilePath,
+                       sizeof(FullModuleFilePath));
+    LogCommentFmt(L"WARP driver loaded from: %ls", FullModuleFilePath);
+  }


This behaves differently than it did previously. The old version very carefully made sure to free the library it had loaded, so make sure that the path it gets from GetModuleHandle refers to the module that's being kept loaded by the device. This has been lost in this change.

I've reverted to identical inline logic.

damyanp · 2025-12-08T21:03:24Z

tools/clang/unittests/HLSLExec/HlslExecTestUtils.cpp

+  // Load WARP DLL if specified
+  WarpDllLoader warpLoader;
+  warpLoader.LoadWarpDll();


FWIW, I think it was clearer and simpler and less lines of code to have this little one-use RAII helper inline here.

Reverted to that.

damyanp

LGTM, although I think this would be improved by having a createHardwareDevice thing as mentioned in my comment.

tex3d

Minor comments.

tex3d · 2026-01-13T02:05:49Z

tools/clang/unittests/HLSLExec/HlslExecTestUtils.cpp

+    if (SkipUnsupported)
+      WEX::Logging::Log::Result(WEX::Logging::TestResults::Skipped);
+
+    D3DDevice = nullptr;


It would be customary to set this to nullptr at the beginning of the function. But also, shouldn't it be *D3DDevice = nullptr;? Otherwise, what's the point?

Also, if you're trying to clear a potentially initialized D3DDevice, I don't think that should be the case when it fails the create, so I don't think you should need to clear it here.

Agreed. Changing this logic to instead use a local CComPtr and detach from that to assign a valid value to the output when it succeeds.

tex3d · 2026-01-13T02:10:11Z

tools/clang/unittests/HLSLExec/HlslExecTestUtils.cpp

+    IDXGIFactory4 *DXGIFactory,
+    std::function<HRESULT(IUnknown *, D3D_FEATURE_LEVEL, REFIID, void **)>
+        CreateDeviceFn,
+    ID3D12Device **D3DDevice) {


Should we clear device on entry here too (*D3DDevice = nullptr)?

tex3d · 2026-01-13T02:14:44Z

I also included a suggestion from co-pilot to add a small RAII wrapper for managing the lifetime of the Warp DLL.

Strange, this seems to imply that struct WarpDll is new, but it looks like it's already in the previous version of the code.

Factor out createWARPDevice and add RAII wrapper for WARP dll.

4c76c65

github-project-automation bot added this to HLSL Roadmap Dec 6, 2025

github-project-automation bot moved this to New in HLSL Roadmap Dec 6, 2025

alsepkow marked this pull request as draft December 6, 2025 00:50

alsepkow added 6 commits December 5, 2025 16:51

formatting

dbef843

A little more cleanup

93b89b7

logAdapter helper

e68145f

Formatting again

ab84cc8

Fix if

fda1eb3

make createWarpDevice return void

198b9f3

alsepkow marked this pull request as ready for review December 8, 2025 20:54

damyanp requested changes Dec 8, 2025

View reviewed changes

github-project-automation bot moved this from New to In progress in HLSL Roadmap Dec 8, 2025

alsepkow added 4 commits December 8, 2025 13:08

Comments

e79ebdd

Move dll loader back to inline

e2231fa

Clang format

ce77e61

Some formatting and remove a stale comment

c687290

damyanp approved these changes Dec 8, 2025

View reviewed changes

alsepkow added 2 commits December 8, 2025 14:23

add createHardwareDevice

1dd271e

Fix createHardwareDevice

a43b082

alsepkow changed the title ~~Execution Tests: Clean up - Factor out createWARPDevice and add RAII wrapper for WARP dll.~~ Execution Tests: Clean up - Factor out createWARPDevice and createHardwareDevice Dec 8, 2025

tex3d reviewed Jan 13, 2026

View reviewed changes

alsepkow and others added 2 commits January 14, 2026 16:07

Assign to nullptr at beginning of helpers in similar fashion to others

b2531d6

chore: autopublish 2026-01-15T00:11:38Z

2dfd48e

Conversation

alsepkow commented Dec 6, 2025

Uh oh!

github-actions bot commented Dec 6, 2025 • edited by alsepkow Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

damyanp commented Dec 8, 2025

Uh oh!

alsepkow commented Dec 8, 2025 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

damyanp left a comment

Choose a reason for hiding this comment

Uh oh!

tex3d left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tex3d commented Jan 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions bot commented Dec 6, 2025 •

edited by alsepkow

Loading