[linux] Enable Reflection on *nix platforms by pow2clk · Pull Request #4810 · microsoft/DirectXShaderCompiler

Gregory Roth (pow2clk) · 2022-11-21T23:51:40Z

This adds the DirectX-Headers repo as a submodule to grant the shader reflection structs
that are needed to enable the functionality on non-windows platforms. No real
changes related to the reflection code had to be made so much as the newly enabled
code had to be made to work on the platform compilers, namely clang. These changes
come first with one separated out for a particularly gnarly issue with a circular dependency
involving the RDAT dumping code.

Thereafter, the submodule is added, the reflection code is enabled for functionality and
tests and a small incidental improvement to error reporting when the data dir isn't found
was made because it's driven me crazy one too many times.

Fixes #4358

Needed for the impending enabling on *nix platforms

A file included by the source that implemented various ToString converters of reflection data to strings itself depended on those declarations, which it only found because of the unique way MSVC processes templates. By moving the definitions and declarations into their own header and source files, everyone gets what they need without ouroborosing.

Creates the DirectX-Headers submodule and adds the needed defines to allow them to be used. As yet makes no use of these headers

The check for the file stream that is meant to represent the filechecked source file wasn't sufficient to detect a non-existent file. By expanding the error check, it can produce a helpful message. Also initialize the failure code so it isn't random

This flips the switch, removing or repositioning many ifdefs and including previously excluded files in the build.

Gregory Roth (pow2clk) · 2022-11-22T01:20:48Z

Note that this is meant to be reviewed commit by commit

AppVeyorBot · 2022-11-22T11:23:03Z

❌ Build DirectXShaderCompiler 1.0.2269 failed (commit 9968374372 by Gregory Roth (@pow2clk))

AppVeyorBot · 2022-11-22T13:52:09Z

❌ Build DirectXShaderCompiler 1.0.2271 failed (commit fbc8f9aff5 by Gregory Roth (@pow2clk))

Chris B (llvm-beanz)

The vast majority of this looks very straightforward to me.

Chris B (llvm-beanz) · 2022-11-22T18:50:25Z

+      CACHE STRING "Location of DirectX-Headers source")
+  if (NOT DEFINED DirectX-Headers_SOURCE_DIR)
+    if (IS_DIRECTORY ${DXC_DIRECTX_HEADERS_DIR})
+      add_subdirectory(${DXC_DIRECTX_HEADERS_DIR}


Do we actually use any of the targets created by the subproject or do we just use the static headers?

If we just use the static headers we should instead be able to just have our FindD3D12 CMake module fall back to the submodule headers. That should simplify the CMake logic.

Currently we do not. However, I greatly prefer the approach that DirectX-Headers takes to defining dx guids as it allows the same guid structure to have the same address across multiple files which ours does not.

I chose not to take that step with this change, but I want to take it going forward when I try to better consolidate the efforts to make non-Windows platforms look a bit more like Windows for DirectX purposes, which is a goal both projects share.

Their GUID handling is all header defined too, right? The source for it just sets a single preprocessor define then includes headers.

If we want to migrate to use it we could define that ourselves, or we depend on their static archive target. The concern I have about their static archive target is that it means filtering target dependencies through the other CMake targets. If we're only doing that on Linux it feels like we might be introducing some extra complexity. Would we shift to using the submodule headers on Windows too?

I mean to shift to using submodule headers on Windows as it would garner us certain advantages, not the least of which being easy access to more recent headers.

You make a good point that we could just as easily recreate dxguid.cpp. Regardless of where we land on Windows using these headers, that's a sufficient argument to simplify this.

Chris B (llvm-beanz) · 2022-11-22T19:00:34Z

  _Out_ D3D12_SHADER_INPUT_BIND_DESC *pDesc, PublicAPI api) {
  IFRBOOL(pDesc != nullptr, E_INVALIDARG);
  IFRBOOL(ResourceIndex < m_Resources.size(), E_INVALIDARG);
+#ifdef _WIN32


splitting a conditional like this is pretty gross... Is there no way to make the if statement valid on Linux?

If not, you could make the if return after the memcpy, which would eliminate the need for an else block.

Chris B (llvm-beanz) · 2022-11-22T19:00:46Z

      }
-      else {
+      else
+#endif // _WIN32


Same here :(

Chris B (llvm-beanz)

I think there is a bigger issue with our find_package usage, but this change all looks good to me. We can sort out the find_package issues later.

…o linflection

AppVeyorBot · 2022-11-22T22:49:38Z

✅ Build DirectXShaderCompiler 1.0.2279 completed (commit 126748665a by Gregory Roth (@pow2clk))

AppVeyorBot · 2022-11-23T00:53:58Z

❌ Build DirectXShaderCompiler 1.0.2281 failed (commit f85bbbe6e8 by Gregory Roth (@pow2clk))

What compiler is overly permissive now huh? huh? (or just quick to adopt new language features?)

AppVeyorBot · 2022-11-23T05:38:34Z

✅ Build DirectXShaderCompiler 1.0.2286 completed (commit 4f2a084d32 by Gregory Roth (@pow2clk))

Loathe as I am to push one more commit onto this. I don't want to propagate a misconception. I meant to remove this earlier and caught it from one final self-review >:(

AppVeyorBot · 2022-11-23T08:38:36Z

✅ Build DirectXShaderCompiler 1.0.2289 completed (commit 6bd150e02e by Gregory Roth (@pow2clk))

AppVeyorBot · 2022-11-23T09:35:15Z

✅ Build DirectXShaderCompiler 1.0.2290 completed (commit 75fa6db0ea by Gregory Roth (@pow2clk))

AppVeyorBot · 2022-11-23T10:35:41Z

✅ Build DirectXShaderCompiler 1.0.2291 completed (commit 7b368d6d5a by Gregory Roth (@pow2clk))

Marijn Suijten (MarijnS95) · 2022-11-28T17:40:07Z

+// need to disable this as it is voilated by this header
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wnon-virtual-dtor"


Gregory Roth (@pow2clk) Didn't we disable this warning altogether as discussed in #3793 (review), or is it now enabled by default on newer compilers (since I only commented out, instead of leaving an unconditional -Wnon-virtual-dtor)?

Marijn Suijten (MarijnS95) · 2022-11-28T17:40:28Z

+#pragma once
+
+#ifndef _WIN32
+// need to disable this as it is voilated by this header


Suggested change

// need to disable this as it is voilated by this header

// need to disable this as it is violated by this header

Marijn Suijten (MarijnS95) · 2022-11-28T17:41:09Z

Thanks for this! Tested on Linux and works a treat, at least for reflecting compute group sizes which we previously couldn't to on our Linux build system!

Greg Roth and others added 5 commits November 21, 2022 14:40

Allow reflection code to compile with Clang

b0e8aea

Needed for the impending enabling on *nix platforms

Create DirectX-Headers submodule

aa21b32

Creates the DirectX-Headers submodule and adds the needed defines to allow them to be used. As yet makes no use of these headers

Improve src dir finding error

0a31264

The check for the file stream that is meant to represent the filechecked source file wasn't sufficient to detect a non-existent file. By expanding the error check, it can produce a helpful message. Also initialize the failure code so it isn't random

Enable reflection functionality and testing

04a9d5f

This flips the switch, removing or repositioning many ifdefs and including previously excluded files in the build.

Gregory Roth (pow2clk) requested review from Chris B (llvm-beanz) and Tex Riddell (tex3d) November 21, 2022 23:51

Actually add the DirectX-Headers submodule

c360ae6

Chris B (llvm-beanz) reviewed Nov 22, 2022

View reviewed changes

Gregory Roth (pow2clk) added 3 commits November 22, 2022 13:34

fix *nix build breaks caused while fixing win breaks :P

09deade

Respond to feedback -- some of it anyway ;)

62b6620

simplify cmake file to exclude unneeded src dir

85f6d58

Chris B (llvm-beanz) approved these changes Nov 22, 2022

View reviewed changes

Merge branch 'main' of github.com:microsoft/DirectXShaderCompiler int…

da6cc33

…o linflection

fix cmake (again)

7095703

Fix GCC template specialization finickiness

d87e286

What compiler is overly permissive now huh? huh? (or just quick to adopt new language features?)

LPCBYTE isn't a thing

83bd996

Loathe as I am to push one more commit onto this. I don't want to propagate a misconception. I meant to remove this earlier and caught it from one final self-review >:(

Gregory Roth (pow2clk) merged commit 14a55b7 into microsoft:main Nov 24, 2022

Gregory Roth (pow2clk) deleted the linflection branch November 24, 2022 02:10

Marijn Suijten (MarijnS95) reviewed Nov 28, 2022

View reviewed changes

Gregory Roth (pow2clk) mentioned this pull request Sep 21, 2023

Plans to support DXIL reflection on non-windows platforms? #2912

Closed

	// need to disable this as it is voilated by this header
	// need to disable this as it is violated by this header

Uh oh!

Conversation

Gregory Roth (pow2clk) commented Nov 21, 2022

Uh oh!

Gregory Roth (pow2clk) commented Nov 22, 2022

Uh oh!

AppVeyorBot commented Nov 22, 2022

Uh oh!

AppVeyorBot commented Nov 22, 2022

Uh oh!

Chris B (llvm-beanz) 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!

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!

Chris B (llvm-beanz) left a comment

Choose a reason for hiding this comment

Uh oh!

AppVeyorBot commented Nov 22, 2022

Uh oh!

AppVeyorBot commented Nov 23, 2022

Uh oh!

AppVeyorBot commented Nov 23, 2022

Uh oh!

AppVeyorBot commented Nov 23, 2022

Uh oh!

AppVeyorBot commented Nov 23, 2022

Uh oh!

AppVeyorBot commented Nov 23, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Marijn Suijten (MarijnS95) commented Nov 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants