Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+15 −5
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1f20688
[NFC] Edit shader flag comments for clarity
Icohedron Mar 28, 2025
151e318
Add a comment to describe the bitmask and to refer to GetFeatureInfo(…
Icohedron Mar 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion include/dxc/DXIL/DxilConstants.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ inline bool UpdateToMaxOfVersions(unsigned &major, unsigned &minor,
return false;
}

// Shader flags.
// Global shader flags
const unsigned kDisableOptimizations =
0x00000001; // D3D11_1_SB_GLOBAL_FLAG_SKIP_OPTIMIZATION
const unsigned kDisableMathRefactoring =
Expand Down
18 changes: 14 additions & 4 deletions include/dxc/DXIL/DxilShaderFlags.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,24 +220,36 @@ class ShaderFlags {
bool GetRequiresGroup() const { return m_bRequiresGroup; }

private:
// Each of the shader flags are defined below within a bitfield to form the
// shader flag bitmask of a DXIL module.
// These shader flags are also used to set Shader Feature Info flags via the
// 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
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

@Icohedron Icohedron Apr 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@Icohedron Icohedron Apr 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It has the ~ negation

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's a typo in DxbcConverter.cpp

unsigned
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
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DxilShaderFlags.cpp:L57

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
Copy link
Collaborator Author

@Icohedron Icohedron Mar 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DxilShaderFlags.cpp:L215

// AND ~m_bUseNativeLowPrecision => SHADER_FEATURE_MINIMUM_PRECISION
// AND m_bUseNativeLowPrecision => SHADER_FEATURE_NATIVE_LOW_PRECISION
Comment on lines +246 to +247
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DxilShaderFlags.cpp:L58
DxilShaderFlags.cpp:L61

unsigned
m_bEnableDoubleExtensions : 1; // D3D11_1_SB_GLOBAL_FLAG_ENABLE_DOUBLE_EXTENSIONS
// SHADER_FEATURE_11_1_DOUBLE_EXTENSIONS
Copy link
Collaborator Author

@Icohedron Icohedron Mar 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DxilShaderFlags.cpp:L65

unsigned m_bEnableMSAD : 1; // D3D11_1_SB_GLOBAL_FLAG_ENABLE_SHADER_EXTENSIONS
// SHADER_FEATURE_11_1_SHADER_EXTENSIONS
Copy link
Collaborator Author

@Icohedron Icohedron Mar 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DxilShaderFlags.cpp:L79


// Bit: 8
unsigned m_bAllResourcesBound : 1; // D3D12_SB_GLOBAL_FLAG_ALL_RESOURCES_BOUND
Expand All @@ -253,8 +265,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
Copy link
Collaborator Author

@Icohedron Icohedron Mar 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

unsigned m_b64UAVs : 1; // SHADER_FEATURE_64_UAVS

// Bit: 16
Expand All @@ -277,7 +287,7 @@ class ShaderFlags {
unsigned m_bBarycentrics : 1; // SHADER_FEATURE_BARYCENTRICS

// SM 6.2+
unsigned m_bUseNativeLowPrecision : 1;
unsigned m_bUseNativeLowPrecision : 1; // see m_bLowPrecisionPresent for uses

// SM 6.4+
// Bit: 24
Expand Down
Loading