[DirectX backend] emits metadata for DXIL version. #88350

python3kgae · 2024-04-11T04:14:25Z

Emit named metadata "dx.version" for DXIL version.

Default to DXIL 1.0

Fixes #88774

llvmbot · 2024-04-11T04:14:54Z

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-backend-directx

Author: Xiang Li (python3kgae)

Changes

Emit named metadata "dx.version" for DXIL version.

Default to DXIL 1.0

Full diff: https://github.com/llvm/llvm-project/pull/88350.diff

5 Files Affected:

(modified) llvm/lib/Target/DirectX/DXILMetadata.cpp (+12)
(modified) llvm/lib/Target/DirectX/DXILMetadata.h (+1)
(modified) llvm/lib/Target/DirectX/DXILTranslateMetadata.cpp (+1)
(added) llvm/test/CodeGen/DirectX/Metadata/dxilVer-1.0.ll (+12)
(added) llvm/test/CodeGen/DirectX/Metadata/dxilVer-1.8.ll (+12)

diff --git a/llvm/lib/Target/DirectX/DXILMetadata.cpp b/llvm/lib/Target/DirectX/DXILMetadata.cpp
index 2d94490a7f24c3..61882f85095664 100644
--- a/llvm/lib/Target/DirectX/DXILMetadata.cpp
+++ b/llvm/lib/Target/DirectX/DXILMetadata.cpp
@@ -81,6 +81,18 @@ void dxil::createShaderModelMD(Module &M) {
   Entry->addOperand(MDNode::get(Ctx, Vals));
 }
 
+void dxil::createDXILVersionMD(Module &M) {
+  Triple TT(M.getTargetTriple());
+  VersionTuple Ver = TT.getOSVersion();
+  LLVMContext &Ctx = M.getContext();
+  IRBuilder<> B(Ctx);
+  NamedMDNode *Entry = M.getOrInsertNamedMetadata("dx.version");
+  Metadata *Vals[2];
+  Vals[0] = ConstantAsMetadata::get(B.getInt32(1));
+  Vals[1] = ConstantAsMetadata::get(B.getInt32(Ver.getMinor().value_or(0)));
+  Entry->addOperand(MDNode::get(Ctx, Vals));
+}
+
 static uint32_t getShaderStage(Triple::EnvironmentType Env) {
   return (uint32_t)Env - (uint32_t)llvm::Triple::Pixel;
 }
diff --git a/llvm/lib/Target/DirectX/DXILMetadata.h b/llvm/lib/Target/DirectX/DXILMetadata.h
index 2f5d7d9fe7683d..c105c15f3d47af 100644
--- a/llvm/lib/Target/DirectX/DXILMetadata.h
+++ b/llvm/lib/Target/DirectX/DXILMetadata.h
@@ -33,6 +33,7 @@ class ValidatorVersionMD {
 };
 
 void createShaderModelMD(Module &M);
+void createDXILVersionMD(Module &M);
 void createEntryMD(Module &M, const uint64_t ShaderFlags);
 
 } // namespace dxil
diff --git a/llvm/lib/Target/DirectX/DXILTranslateMetadata.cpp b/llvm/lib/Target/DirectX/DXILTranslateMetadata.cpp
index 80d94bf0c9d488..ae6d6f96904c86 100644
--- a/llvm/lib/Target/DirectX/DXILTranslateMetadata.cpp
+++ b/llvm/lib/Target/DirectX/DXILTranslateMetadata.cpp
@@ -48,6 +48,7 @@ bool DXILTranslateMetadata::runOnModule(Module &M) {
   if (ValVerMD.isEmpty())
     ValVerMD.update(VersionTuple(1, 0));
   dxil::createShaderModelMD(M);
+  dxil::createDXILVersionMD(M);
 
   const dxil::Resources &Res =
       getAnalysis<DXILResourceWrapper>().getDXILResource();
diff --git a/llvm/test/CodeGen/DirectX/Metadata/dxilVer-1.0.ll b/llvm/test/CodeGen/DirectX/Metadata/dxilVer-1.0.ll
new file mode 100644
index 00000000000000..254479e5f94cbd
--- /dev/null
+++ b/llvm/test/CodeGen/DirectX/Metadata/dxilVer-1.0.ll
@@ -0,0 +1,12 @@
+; RUN: opt -S -dxil-metadata-emit %s | FileCheck %s
+target triple = "dxil-pc-shadermodel6.0-vertex"
+
+; CHECK: !dx.version = !{![[DXVER:[0-9]+]]}
+; CHECK: ![[DXVER]] = !{i32 1, i32 0}
+
+define void @entry() #0 {
+entry:
+  ret void
+}
+
+attributes #0 = { noinline nounwind "hlsl.shader"="vertex" }
diff --git a/llvm/test/CodeGen/DirectX/Metadata/dxilVer-1.8.ll b/llvm/test/CodeGen/DirectX/Metadata/dxilVer-1.8.ll
new file mode 100644
index 00000000000000..efeb5a1b24862e
--- /dev/null
+++ b/llvm/test/CodeGen/DirectX/Metadata/dxilVer-1.8.ll
@@ -0,0 +1,12 @@
+; RUN: opt -S -dxil-metadata-emit %s | FileCheck %s
+target triple = "dxil-pc-shadermodel6.8-compute"
+
+; CHECK: !dx.version = !{![[DXVER:[0-9]+]]}
+; CHECK: ![[DXVER]] = !{i32 1, i32 8}
+
+define void @entry() #0 {
+entry:
+  ret void
+}
+
+attributes #0 = { noinline nounwind "hlsl.numthreads"="1,2,1" "hlsl.shader"="compute" }

thegreatbeanz · 2024-04-11T20:25:30Z

llvm/lib/Target/DirectX/DXILMetadata.cpp

@@ -81,6 +81,18 @@ void dxil::createShaderModelMD(Module &M) {
  Entry->addOperand(MDNode::get(Ctx, Vals));
 }

+void dxil::createDXILVersionMD(Module &M) {
+  Triple TT(M.getTargetTriple());
+  VersionTuple Ver = TT.getOSVersion();


I think we should reconsider how we're representing the DXIL version. In DXC we've always made the DXIL version and Shader Model version lockstep and we've made a lot of assumptions about that.

I suggested in a separate chat channel that we should consider adding DXIL versions as sub-architectures.

I'm curious for feedback from @bharadwajy and @bogner.

What are the anticipated benefits of separating the Shader Model version number and DXIL version number, given future plans for the evolution of HLSL and DXIL?

Apologies for any confusion caused by me posting the comment from an alternate account (@thegreatbeanz) is me.

I can't share anything about future plans for HLSL or DXIL, aside from what is already public. That said, I don't think we need to consider future plans to see the potential benefit of keeping DXIL and SM versions separate and clearly defined.

We haven't done a great job delineating shader model versions from DXIL versions in DXC, but they are different. A good example is the quadVote DXIL Op (described here). While the quadVote DXIL Op was added in DXIL 1.7, the QuadAny/QuadAll HLSL intrinsics were retroactively added back to Shader Model 6.0 because they can be lowered to different DXIL operations on older DXIL versions. We've also done this for other new functions like the IsHelperLane intrinsic added in Shader Model 6.6.

This means there isn't a 1:1 mapping between source-level intrinsics where shader model is our versioning, and DXIL operations where the DXIL version is our mechanism.

When we think about expanding DXIL targeting support to other languages, it is probably more clear to separate the Shader Model version (which is an HLSL concept) from the DXIL version (which is a code-generation concept).

This means there isn't a 1:1 mapping between source-level intrinsics where shader model is our versioning, and DXIL operations where the DXIL version is our mechanism.

When we think about expanding DXIL targeting support to other languages, it is probably more clear to separate the Shader Model version (which is an HLSL concept) from the DXIL version (which is a code-generation concept).

This sounds wrong. Shader Model is specifically a target constraint concept applying to the features used in the IR we pass to drivers. DXIL version is an IR versioning concept. In the past, if something in HLSL could be mapped to the constraints in a shader model by the compiler, then it was legal to use in that shader model. It was never considered an HLSL concept/constraint. We may want to put constraints on HLSL APIs in the future based on the shader model used, but that is not the core role of the shader model.

We also add things to the language/API in shader model time frames, but that doesn't mean that using those things at the source level always requires compilation to those shader models. They just require a new enough compiler that has the source-level construct (like QuaAny/QuadAll/IsHelperLane).

In the past, they have advanced in lock-step because with each new shader model, we have needed updates to DXIL, making it necessary to advance that version, and we can't use a newer DXIL version for an older shader model, since drivers reporting support for a particular shader model are not expected to support any newer DXIL versions beyond that shader model.

Issue #91388 is created to track validation of dxil version and shader model.

bogner

If the fact that this is missing is blocking something then it's probably fine to do it like this for now, but as implied by #66015 and #66016 we'll presumably need to revisit this as we evolve our story around DXIL metadata abstractions.

In any case, it's probably worth considering @thegreatbeanz's point about separating the abstraction of DXIL version from the shader model. It'll be much easier to decouple these if we design that early rather than if we bake in assumptions like this in the short term.

damyanp · 2024-04-15T18:56:28Z

Default to DXIL 1.0

Can you clarify what the intent of this PR is please? It looks like it is actually trying to match current DXC behavior that assumes that the DXIL version is tied to the SM version?

python3kgae · 2024-04-15T19:30:47Z

Default to DXIL 1.0

Can you clarify what the intent of this PR is please? It looks like it is actually trying to match current DXC behavior that assumes that the DXIL version is tied to the SM version?

The intent for this PR is to match current DXC behavior when DXIL version is not set separately.
Once we have DXIL version sent to backend with Triple or something else, we could use that in another PR.

Emit named metadata "dx.version". Default to DXIL 1.0

github-actions · 2024-05-07T16:42:40Z

✅ With the latest revision this PR passed the C/C++ code formatter.

llvm-beanz · 2024-05-07T16:52:28Z

llvm/lib/Transforms/Utils/DXIL.cpp

+}
+
+Expected<DXILVersion> DXILVersion::get(Module &M) {
+  Triple TT(Triple::normalize(M.getTargetTriple()));


Normalizing the triple should be sufficient for identifying the sub architecture. @bharadwajy's PR (#90809) needed to be reverted, but when it re-lands this should radically simplify.

bogner · 2024-05-07T19:04:59Z

I think we should probably just do the simpler dxil::createDXILVersionMD version of this for now, rather than adding the DXILVersion abstraction. The DXILVersion abstraction is similar to the prototype shader model abstraction in #66015, but having thought about what I was doing there for a bit I think it's overengineered and won't scale very well to the rest of the stuff we store in metadata.

Longer term I'd like to go the route of #63835, where we introduce a metadata analysis pass that generates some kind of DXILMetadata type that can abstract over the metadata stuff a bit more simply.

This reverts commit 85c007f.

This reverts commit 6d0a795.

bogner · 2024-05-07T20:48:32Z

llvm/include/llvm/TargetParser/Triple.h

@@ -429,6 +429,10 @@ class Triple {
  /// (SubArch).  This should only be called with Vulkan SPIR-V triples.
  VersionTuple getVulkanVersion() const;

+  /// Parse the DXIL version number from the OSVersion and DXIL version
+  /// (SubArch).  This should only be called with DXIL triples.
+  VersionTuple getDXILVersion() const;


This comment doesn't seem accurate, given that getDXILVersion doesn't currently use the OS version

bogner · 2024-05-07T20:49:29Z

llvm/lib/TargetParser/Triple.cpp

+  switch (getSubArch()) {
+  case Triple::NoSubArch:
+  case Triple::DXILSubArch_v1_0:
+    DXILVersion = VersionTuple(1, 0);


Can this be simplified using ParseVersionFromName?

bharadwajy · 2024-05-08T13:35:33Z

llvm/lib/Target/DirectX/DXILMetadata.h

@@ -33,6 +33,7 @@ class ValidatorVersionMD {
 };

 void createShaderModelMD(Module &M);
+void createDXILVersionMD(Module &M);


Consider expanding MD as Metadata in the function name createDXILVersionMD as createDXILVersionMetadata. Similarly, createShaderModelMetadata.

llvmbot added the backend:DirectX label Apr 11, 2024

thegreatbeanz reviewed Apr 11, 2024

View reviewed changes

bogner reviewed Apr 15, 2024

View reviewed changes

This was referenced Apr 16, 2024

[DXIL] Separate versioned specifications of DXIL and HLSL Shader Model #88885

Closed

[DXIL] Add DXIL SubArch to correspond to version number #89125

Merged

python3kgae added 2 commits May 6, 2024 18:35

[DirectX backend] emit metadata for DXIL version.

d9353d1

Emit named metadata "dx.version". Default to DXIL 1.0

Get dxil version from subarch.

2b13ac2

python3kgae force-pushed the dx_ver_part branch from 50e271c to 2b13ac2 Compare May 6, 2024 22:50

Add DXILVersion for DXIL metadata abstractions

6d0a795

llvmbot added the llvm:transforms label May 7, 2024

Fix clang-format

85c007f

llvm-beanz reviewed May 7, 2024

View reviewed changes

python3kgae added 4 commits May 7, 2024 15:56

Revert "Fix clang-format"

889332e

This reverts commit 85c007f.

Revert "Add DXILVersion for DXIL metadata abstractions"

8252881

This reverts commit 6d0a795.

Add Triple::getDXILVersion.

de415cd

Fix clang-format.

6581b3d

bogner reviewed May 7, 2024

View reviewed changes

Use parseVersionFromName.

21dd277

bogner approved these changes May 7, 2024

View reviewed changes

bharadwajy approved these changes May 8, 2024

View reviewed changes

python3kgae merged commit 665af09 into llvm:main May 8, 2024
3 of 4 checks passed

python3kgae deleted the dx_ver_part branch May 8, 2024 13:40

bogner mentioned this pull request May 14, 2024

[DirectX] Introduce abstraction for translating shader model between triple representation and DXIL metadata #66015

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DirectX backend] emits metadata for DXIL version. #88350

[DirectX backend] emits metadata for DXIL version. #88350

python3kgae commented Apr 11, 2024 •

edited

llvmbot commented Apr 11, 2024 •

edited

thegreatbeanz Apr 11, 2024

bharadwajy Apr 12, 2024 •

edited

llvm-beanz Apr 15, 2024

tex3d May 7, 2024

python3kgae May 7, 2024

bogner left a comment

damyanp commented Apr 15, 2024

python3kgae commented Apr 15, 2024

github-actions bot commented May 7, 2024 •

edited

llvm-beanz May 7, 2024

bogner commented May 7, 2024

bogner May 7, 2024

python3kgae May 7, 2024

bogner May 7, 2024

python3kgae May 7, 2024

bharadwajy May 8, 2024

[DirectX backend] emits metadata for DXIL version. #88350

[DirectX backend] emits metadata for DXIL version. #88350

Conversation

python3kgae commented Apr 11, 2024 • edited

llvmbot commented Apr 11, 2024 • edited

Choose a reason for hiding this comment

bharadwajy Apr 12, 2024 • edited

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

bogner left a comment

Choose a reason for hiding this comment

damyanp commented Apr 15, 2024

python3kgae commented Apr 15, 2024

github-actions bot commented May 7, 2024 • edited

Choose a reason for hiding this comment

bogner commented May 7, 2024

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

Choose a reason for hiding this comment

python3kgae commented Apr 11, 2024 •

edited

llvmbot commented Apr 11, 2024 •

edited

bharadwajy Apr 12, 2024 •

edited

github-actions bot commented May 7, 2024 •

edited