[NFC] Edit DXIL module flag comments for clarity #7271

Icohedron · 2025-03-28T19:21:12Z

Some of the comments in DxilShaderFlags.h are incomplete or misleading.

This PR edits the comments in DxilShaderFlags.h to be more clear as to which Shader Feature Info Flags and Global Flags each Module Flag is used to set.

In the case of the m_bLowPrecisionPresent module flag, it used to be the case that this implied minimum precision. However, as of today, low precision present is a necessary but not sufficient condition for minimum precision due to the existence of native low precision (m_bUseNativeLowPrecision). The comment for m_bLowPrecisionPresent has been rewritten to clarify this.

github-actions · 2025-03-28T19:22:05Z

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

You can test this locally with the following command:

git-clang-format --diff 5ff9cbc7cb83ab2d5f52255412f61ac3226c4a08 151e31892cc475d5c581b8f344c30b6102c46d64 -- include/dxc/DXIL/DxilConstants.h include/dxc/DXIL/DxilShaderFlags.h

View the diff from clang-format here.

diff --git a/include/dxc/DXIL/DxilShaderFlags.h b/include/dxc/DXIL/DxilShaderFlags.h
index 06b50f62..b0cfd278 100644
--- a/include/dxc/DXIL/DxilShaderFlags.h
+++ b/include/dxc/DXIL/DxilShaderFlags.h
@@ -242,9 +242,12 @@ private:
   unsigned
       m_bEnableRawAndStructuredBuffers : 1; // D3D11_SB_GLOBAL_FLAG_ENABLE_RAW_AND_STRUCTURED_BUFFERS
   unsigned
-      m_bLowPrecisionPresent : 1; // AND ~m_bUseNativeLowPrecision => D3D11_1_SB_GLOBAL_FLAG_ENABLE_MINIMUM_PRECISION
-                                  // AND ~m_bUseNativeLowPrecision => SHADER_FEATURE_MINIMUM_PRECISION
-                                  // AND m_bUseNativeLowPrecision => SHADER_FEATURE_NATIVE_LOW_PRECISION
+      m_bLowPrecisionPresent : 1; // AND ~m_bUseNativeLowPrecision =>
+                                  // D3D11_1_SB_GLOBAL_FLAG_ENABLE_MINIMUM_PRECISION
+                                  // AND ~m_bUseNativeLowPrecision =>
+                                  // SHADER_FEATURE_MINIMUM_PRECISION AND
+                                  // m_bUseNativeLowPrecision =>
+                                  // SHADER_FEATURE_NATIVE_LOW_PRECISION
   unsigned
       m_bEnableDoubleExtensions : 1; // D3D11_1_SB_GLOBAL_FLAG_ENABLE_DOUBLE_EXTENSIONS
                                      // SHADER_FEATURE_11_1_DOUBLE_EXTENSIONS

Check this box to apply formatting changes to this branch.

Icohedron · 2025-03-28T19:22:30Z

include/dxc/DXIL/DxilShaderFlags.h

  unsigned m_bEnableMSAD : 1; // D3D11_1_SB_GLOBAL_FLAG_ENABLE_SHADER_EXTENSIONS
+                              // SHADER_FEATURE_11_1_SHADER_EXTENSIONS


DxilShaderFlags.cpp:L79

Icohedron · 2025-03-28T19:22:58Z

include/dxc/DXIL/DxilShaderFlags.h

  unsigned
      m_bEnableDoubleExtensions : 1; // D3D11_1_SB_GLOBAL_FLAG_ENABLE_DOUBLE_EXTENSIONS
+                                     // SHADER_FEATURE_11_1_DOUBLE_EXTENSIONS


DxilShaderFlags.cpp:L65

Icohedron · 2025-03-28T19:23:40Z

include/dxc/DXIL/DxilShaderFlags.h

@@ -227,17 +227,22 @@ class ShaderFlags {
      m_bDisableMathRefactoring : 1; //~D3D10_SB_GLOBAL_FLAG_REFACTORING_ALLOWED
  unsigned
      m_bEnableDoublePrecision : 1; // D3D11_SB_GLOBAL_FLAG_ENABLE_DOUBLE_PRECISION_FLOAT_OPS
+                                    // SHADER_FEATURE_DOUBLES


DxilShaderFlags.cpp:L57

Icohedron · 2025-03-28T19:25:25Z

include/dxc/DXIL/DxilShaderFlags.h

@@ -253,8 +258,6 @@ class ShaderFlags {
      m_bUAVLoadAdditionalFormats : 1; // SHADER_FEATURE_TYPED_UAV_LOAD_ADDITIONAL_FORMATS
  unsigned
      m_bLevel9ComparisonFiltering : 1; // SHADER_FEATURE_LEVEL_9_COMPARISON_FILTERING
-                                        // SHADER_FEATURE_11_1_SHADER_EXTENSIONS
-                                        // shared with EnableMSAD


I removed these two lines because I see m_bLevel9ComparisonFiltering only being used to set SHADER_FEATURE_LEVEL_9_COMPARISON_FILTERING.
DxilShaderFlags.cpp:L88

Icohedron · 2025-03-28T19:26:22Z

include/dxc/DXIL/DxilShaderFlags.h

  unsigned
      m_bForceEarlyDepthStencil : 1; // D3D11_SB_GLOBAL_FLAG_FORCE_EARLY_DEPTH_STENCIL

  // Bit: 4
  unsigned
      m_bEnableRawAndStructuredBuffers : 1; // D3D11_SB_GLOBAL_FLAG_ENABLE_RAW_AND_STRUCTURED_BUFFERS
  unsigned
-      m_bLowPrecisionPresent : 1; // D3D11_1_SB_GLOBAL_FLAG_ENABLE_MINIMUM_PRECISION
+      m_bLowPrecisionPresent : 1; // AND ~m_bUseNativeLowPrecision => D3D11_1_SB_GLOBAL_FLAG_ENABLE_MINIMUM_PRECISION


DxilShaderFlags.cpp:L215

Icohedron · 2025-03-28T19:27:53Z

include/dxc/DXIL/DxilShaderFlags.h

+                                  // AND ~m_bUseNativeLowPrecision => SHADER_FEATURE_MINIMUM_PRECISION
+                                  // AND m_bUseNativeLowPrecision => SHADER_FEATURE_NATIVE_LOW_PRECISION


DxilShaderFlags.cpp:L58
DxilShaderFlags.cpp:L61

Icohedron · 2025-03-28T19:29:42Z

@microsoft-github-policy-service agree company="Microsoft"

damyanp

TBH I don't find this any easier to follow. I suspect that if a comment said "see ShaderFlags::GetGlobalFlags() to understand how these are used to generate shader flags" then that would be the sign post I'd need to figure this out.

Icohedron · 2025-03-29T00:12:20Z

TBH I don't find this any easier to follow. I suspect that if a comment said "see ShaderFlags::GetGlobalFlags() to understand how these are used to generate shader flags" then that would be the sign post I'd need to figure this out.

The existing comments appeared to be telling what shader feature flag or global flag each of the module flags are equivalent to. For example, setting the module flag m_bEnableDoubleExtensions is equivalent to setting the global flag D3D11_SB_GLOBAL_FLAG_ENABLE_DOUBLE_PRECISION_FLOAT_OPS.
Of course, this commenting scheme broke when m_bLowPrecisionPresent became no longer equivalent to D3D11_1_SB_GLOBAL_FLAG_ENABLE_MINIMUM_PRECISION.

Currently, this PR tries to fix the comments to keep the existing comment scheme, but I guess it does seem rather redundant to have comments that describe implementations that are easy enough to read on their own.

Perhaps all the SHADER_FEATURE_* and *GLOBAL_FLAG* comments should be removed altogether?
Then have one comment to refer to ShaderFlags::GetFeatureInfo() to understand how these module flags are used to generate shader feature info flags, and another comment to refer to ShaderFlags::GetGlobalFlags() to understand how these module flags are used to generate global flags.

…) and GetGlobalFlags() for shader feature and global flags

inbelic · 2025-04-14T19:05:23Z

include/dxc/DXIL/DxilShaderFlags.h

+  // function ShaderFlags::GetFeatureInfo().
+  // For DXBC, these shader flags are also used to set Global Flags via the
+  // function ShaderFlags::GetGlobalFlags().
+
  // Bit: 0
  unsigned
      m_bDisableOptimizations : 1; // D3D11_1_SB_GLOBAL_FLAG_SKIP_OPTIMIZATION


These mappings seem to be taken from

DirectXShaderCompiler/projects/dxilconv/lib/DxbcConverter/DxbcConverter.cpp

Line 439 in 8280d0f

void DxbcConverter::SetShaderGlobalFlags(unsigned GlobalFlags) {

?

Worryingly they might not all be consistent (this one for example is not equivalent).

Also, if I search for D3D11_1_SB_GLOBAL_FLAG_SKIP_OPTIMIZATION in this repo and online I don't really get directed to any documentation that is helpful? Maybe I am missing something.

So I might say we could remove these comments and just point to the implementation with your comment?

The best description I can find for D3D11_1_SB_GLOBAL_FLAG_SKIP_OPTIMIZATION is from

DirectXShaderCompiler/include/dxc/Support/d3d12TokenizedProgramFormat.hpp

Line 1194 in 8280d0f

// [15:15] Skip optimizations of shader IL when translating to native code

Which implies that optimizations should be skipped by the backend/driver compiler when translating to native code.

Also, I don't see how the code you linked from DxbcConverter.cpp is inconsistent with these comments in DxilshaderFlags.h

It has the ~ negation

I think it's a typo in DxbcConverter.cpp

[NFC] Edit shader flag comments for clarity

1f20688

Icohedron requested a review from a team as a code owner March 28, 2025 19:21

github-project-automation bot added this to HLSL Roadmap Mar 28, 2025

github-project-automation bot moved this to New in HLSL Roadmap Mar 28, 2025

Icohedron commented Mar 28, 2025

View reviewed changes

damyanp reviewed Mar 28, 2025

View reviewed changes

Add a comment to describe the bitmask and to refer to GetFeatureInfo(…

151e318

…) and GetGlobalFlags() for shader feature and global flags

inbelic reviewed Apr 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[NFC] Edit DXIL module flag comments for clarity #7271

[NFC] Edit DXIL module flag comments for clarity #7271

Uh oh!

Icohedron commented Mar 28, 2025

Uh oh!

github-actions bot commented Mar 28, 2025 •

edited

Loading

Uh oh!

Icohedron Mar 28, 2025 •

edited

Loading

Uh oh!

Icohedron Mar 28, 2025 •

edited

Loading

Uh oh!

Icohedron Mar 28, 2025

Uh oh!

Icohedron Mar 28, 2025 •

edited

Loading

Uh oh!

Icohedron Mar 28, 2025 •

edited

Loading

Uh oh!

Icohedron Mar 28, 2025

Uh oh!

Icohedron commented Mar 28, 2025

Uh oh!

damyanp left a comment

Uh oh!

Icohedron commented Mar 29, 2025

Uh oh!

inbelic Apr 14, 2025

Uh oh!

Icohedron Apr 14, 2025 •

edited

Loading

Uh oh!

Icohedron Apr 14, 2025 •

edited

Loading

Uh oh!

inbelic Apr 14, 2025

Uh oh!

Icohedron Apr 14, 2025

Uh oh!

Uh oh!

		unsigned m_bEnableMSAD : 1; // D3D11_1_SB_GLOBAL_FLAG_ENABLE_SHADER_EXTENSIONS
		// SHADER_FEATURE_11_1_SHADER_EXTENSIONS

		// AND ~m_bUseNativeLowPrecision => SHADER_FEATURE_MINIMUM_PRECISION
		// AND m_bUseNativeLowPrecision => SHADER_FEATURE_NATIVE_LOW_PRECISION

[NFC] Edit DXIL module flag comments for clarity #7271

Are you sure you want to change the base?

[NFC] Edit DXIL module flag comments for clarity #7271

Uh oh!

Conversation

Icohedron commented Mar 28, 2025

Uh oh!

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

Uh oh!

Uh oh!

Icohedron Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Icohedron Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Icohedron Mar 28, 2025

Choose a reason for hiding this comment

Uh oh!

Icohedron Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Icohedron Mar 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Icohedron Mar 28, 2025

Choose a reason for hiding this comment

Uh oh!

Icohedron commented Mar 28, 2025

Uh oh!

damyanp left a comment

Choose a reason for hiding this comment

Uh oh!

Icohedron commented Mar 29, 2025

Uh oh!

inbelic Apr 14, 2025

Choose a reason for hiding this comment

Uh oh!

Icohedron Apr 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Icohedron Apr 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

inbelic Apr 14, 2025

Choose a reason for hiding this comment

Uh oh!

Icohedron Apr 14, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Mar 28, 2025 •

edited

Loading

Icohedron Mar 28, 2025 •

edited

Loading

Icohedron Mar 28, 2025 •

edited

Loading

Icohedron Mar 28, 2025 •

edited

Loading

Icohedron Mar 28, 2025 •

edited

Loading

Icohedron Apr 14, 2025 •

edited

Loading

Icohedron Apr 14, 2025 •

edited

Loading