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

bharadwajy · 2024-04-17T19:27:09Z

This change is in line with similar notation in SPIRV.

Decoupling Version numbering of DXIL and of Shader Model is intended to enable accurate specification of DXIL features that can be targeted by different Shader Model versions. See here for additional context.

github-actions · 2024-04-17T19:30:00Z

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

damyanp · 2024-04-17T19:55:10Z

llvm/lib/TargetParser/Triple.cpp

+          .Case("csky", Triple::csky)
+          .Case("loongarch32", Triple::loongarch32)
+          .Case("loongarch64", Triple::loongarch64)
+          .Case("dxil", Triple::dxil)


I don't really understand what this block does, but I notice that there's a StartsWith("dxil", ...) as well as a Case("dxil", ...). Should they both be there?

How come spirv lists them all out but dxil doesn't?

I don't really understand what this block does, but I notice that there's a StartsWith("dxil", ...) as well as a Case("dxil", ...). Should they both be there?

Merged them.

How come spirv lists them all out but dxil doesn't?

I believe spirv lists them out to distinguish between spirv, spirv32 and spirv64.

I don't really understand what this block does, but I notice that there's a StartsWith("dxil", ...) as well as a Case("dxil", ...). Should they both be there?

Merged them.

How come spirv lists them all out but dxil doesn't?

I believe spirv lists them out to distinguish between spirv, spirv32 and spirv64.

Listing them out appears to be a better option. Pushed the change. Thanks!

damyanp · 2024-04-17T19:55:58Z

llvm/lib/TargetParser/Triple.cpp

+    return StringSwitch<Triple::SubArchType>(SubArchName)
+        .EndsWith("v1.0", Triple::DXILSubArch_v10)
+        .EndsWith("v1.1", Triple::DXILSubArch_v11)
+        .EndsWith("v1.2", Triple::DXILSubArch_v12)


does it matter than this would match something like "dxilborkborkborkv1.2"?

does it matter than this would match something like "dxilborkborkborkv1.2"?

Pushed a change to address this. Thanks!

llvm/unittests/TargetParser/TripleTest.cpp

damyanp · 2024-04-17T20:30:33Z

Is this related to #88885?

bharadwajy · 2024-04-17T20:46:14Z

Is this related to #88885?

Yes.

bogner

What's the plan for when a subarch isn't specified (ie, dxil-pc-shadermodel-6.0)? Will we need a mapping somewhere else that applies a default dxil version for a given shader model? Will we just have to check both the dxil subarch and the shader model anywhere where it matters?

llvm/lib/TargetParser/Triple.cpp

bogner · 2024-04-17T21:29:18Z

llvm/unittests/TargetParser/TripleTest.cpp

+
+  T = Triple("dxilv1.9-unknown-unknown");
+  EXPECT_EQ(Triple::UnknownArch, T.getArch());
+  EXPECT_EQ(Triple::NoSubArch, T.getSubArch());


Probably worth pointing out in a comment that this is a negative test case and that it's intentional that we're checking for an unknown triple here.

If we're going to test an unknown version, maybe we should pick one that isn't likely to happen anytime soon... Since we just released 1.8 and we're publicly working on previews for potential new features, seems like 1.9 could happen imminently. 1.99 seems like it is a long way off though.

bharadwajy · 2024-04-17T21:44:49Z

What's the plan for when a subarch isn't specified (ie, dxil-pc-shadermodel-6.0)? Will we need a mapping somewhere else that applies a default dxil version for a given shader model? Will we just have to check both the dxil subarch and the shader model anywhere where it matters?

I propose defaulting to version 1.0 when SubArch is not explicitly specified.

damyanp · 2024-04-17T22:12:17Z

What's the plan for when a subarch isn't specified (ie, dxil-pc-shadermodel-6.0)?
I propose defaulting to version 1.0 when SubArch is not explicitly specified.

So if I specify dxil-pc-shadermodel-6.8 then the SubArch will be DXILSubArch_v10? Will that work out ok everywhere?

damyanp · 2024-04-17T22:25:05Z

Is this related to #88885?

Yes.

Could you link it to the issue please? Either with a "fixes" note in the description or using the "Development" section on the right.

llvm-beanz · 2024-04-17T22:19:30Z

llvm/include/llvm/TargetParser/Triple.h

+    DXILSubArch_v15,
+    DXILSubArch_v16,
+    DXILSubArch_v17,
+    DXILSubArch_v18,


This is going to get real awkward if we hit v 1.10... maybe we should separate the major and minor versions with _.

llvm-beanz · 2024-04-17T22:26:56Z

llvm/unittests/TargetParser/TripleTest.cpp

+
+  T = Triple("dxilv1.9-unknown-unknown");
+  EXPECT_EQ(Triple::UnknownArch, T.getArch());
+  EXPECT_EQ(Triple::NoSubArch, T.getSubArch());


If we're going to test an unknown version, maybe we should pick one that isn't likely to happen anytime soon... Since we just released 1.8 and we're publicly working on previews for potential new features, seems like 1.9 could happen imminently. 1.99 seems like it is a long way off though.

bharadwajy · 2024-04-17T22:43:50Z

Is this related to #88885?

Yes.

Could you link it to the issue please? Either with a "fixes" note in the description or using the "Development" section on the right.

I linked this branch in "Development" section.

This change is in line with similar notation in SPIRV.

bharadwajy · 2024-04-18T17:20:01Z

What's the plan for when a subarch isn't specified (ie, dxil-pc-shadermodel-6.0)?
I propose defaulting to version 1.0 when SubArch is not explicitly specified.

So if I specify dxil-pc-shadermodel-6.8 then the SubArch will be DXILSubArch_v10? Will that work out ok everywhere?

On second thoughts, it appears that shader model version is embedded in the -T <target profile> option specified on the HLSL compiler command line (e.g., ps_6_0, lib_6_8). Since Shader Model version is being decoupled from DXIL version, the target DXIL version is also required to be explicitly specified in the target triple. This implies dxil-pc-shadermodel-6.8 would be flagged as an error.

damyanp · 2024-04-18T17:51:02Z

What's the plan for when a subarch isn't specified (ie, dxil-pc-shadermodel-6.0)?
I propose defaulting to version 1.0 when SubArch is not explicitly specified.

So if I specify dxil-pc-shadermodel-6.8 then the SubArch will be DXILSubArch_v10? Will that work out ok everywhere?

On second thoughts, it appears that shader model version is embedded in the -T <target profile> option specified on the HLSL compiler command line (e.g., ps_6_0, lib_6_8). Since Shader Model version is being decoupled from DXIL version, the target DXIL version is also required to be explicitly specified in the target triple. This implies dxil-pc-shadermodel-6.8 would be flagged as an error.

Something somewhere will need to validate that the dxil version is compatible with the SM version, right?

I think we're at a point now where we're speccing the feature in a PR for the implementation. Sounds like we need some design discussion before continuing with this.

Change enum value from the form DXILSubArch_v<major><minor> to DXILSubArch_v<major>_<minor> Add comment in test

llvm-beanz · 2024-04-18T18:46:57Z

In the clang DXC driver mode we translate target profiles to triples (i.e. ps_6_6 to dxil-pc-shadermodel6.6-pixel). Clang needs two changes to utilize this:

translate target profile to dxil version and shader model version.
In the non-DXC-mode driver, we should propagate shader model version to dxil sub arch if the DXIL sub arch is not set.

I think these changes are extremely straightforward, I'm not sure why we would need a larger design discussion around it.

damyanp · 2024-04-18T18:50:13Z

In the clang DXC driver mode we translate target profiles to triples (i.e. ps_6_6 to dxil-pc-shadermodel6.6-pixel). Clang needs two changes to utilize this [...]

This explanation helps. I don't recall seeing this written down anywhere before so I've been trying to understand this change without this context, which is why I was asking for more explanation about what's going on.

damyanp · 2024-04-18T19:04:15Z

In the clang DXC driver mode we translate target profiles to triples (i.e. ps_6_6 to dxil-pc-shadermodel6.6-pixel). Clang needs two changes to utilize this:

translate target profile to dxil version and shader model version.

In TripleTest.cpp I don't see any test cases that exercise a specific shadermodel being specified. These are all of the form:

dxil-unknown-shadermodel-amplification
dxilv1.0-unknown-unknown

In the non-DXC-mode driver, we should propagate shader model version to dxil sub arch if the DXIL sub arch is not set.

I can't see anything in this change that does that?

Is this something that should be done in this PR, or is this happening elsewhere?

llvm-beanz · 2024-04-18T19:10:59Z

In TripleTest.cpp I don't see any test cases that exercise a specific shadermodel being specified. These are all of the form:

dxil-unknown-shadermodel-amplification

dxilv1.0-unknown-unknown

We can add those tests, but the LLVM code shouldn't be responsible for this. LLVM should generate the code based on the triple provided or hard error if it can't. That's separate from the shader model version to DXIL version translation which should happen in Clang.

I can't see anything in this change that does that?
Is this something that should be done in this PR, or is this happening elsewhere?

This is the requisite LLVM side of the change. We also will need a change to Clang's driver to implement setting the sub architecture as I described.

We could do that in this PR or separately.

llvm-beanz

I'm happy with this change as it is with one request.

@bharadwajy, if you're not going to do the clang change with this patch, can you file an issue to track that because we will need that change to land before we can start making the backend depend on the sub-architecture (otherwise we're always going to set it wrong).

damyanp

I think this change looks ok to me, but I'd love to have a better understanding of the plan for how these DXIL versions / sub archs / SM versions all work together in clang/clang-dxc/llvm.

bharadwajy · 2024-04-18T21:29:21Z

I'm happy with this change as it is with one request.

@bharadwajy, if you're not going to do the clang change with this patch, can you file an issue to track that because we will need that change to land before we can start making the backend depend on the sub-architecture (otherwise we're always going to set it wrong).

Filed an issue #89316.

bharadwajy requested review from llvm-beanz, sudonatalie and bogner April 17, 2024 19:27

damyanp reviewed Apr 17, 2024

View reviewed changes

llvm/unittests/TargetParser/TripleTest.cpp Show resolved Hide resolved

bogner reviewed Apr 17, 2024

View reviewed changes

llvm-beanz reviewed Apr 17, 2024

View reviewed changes

bharadwajy added 5 commits April 18, 2024 12:09

[DXIL] Add DXIL SubArch to correspond to version number

6ff66c1

This change is in line with similar notation in SPIRV.

Merge dxil comparison per PR feedback.

532fa6d

Add a couple of additional tests per PR feedback

0a6afe0

Spell out all valid DXIL SubArch values.

816021a

Fix code format per git-clang-format

72a033c

damyanp assigned bharadwajy Apr 18, 2024

Incorporate PR feedback.

93a8e55

Change enum value from the form DXILSubArch_v<major><minor> to DXILSubArch_v<major>_<minor> Add comment in test

bharadwajy force-pushed the dxil-subtarget branch from 8cd1f4d to 93a8e55 Compare April 18, 2024 18:32

llvm-beanz approved these changes Apr 18, 2024

View reviewed changes

damyanp approved these changes Apr 18, 2024

View reviewed changes

bharadwajy mentioned this pull request Apr 18, 2024

[DXIL] [Clang] Set DXIL version in target triple when translating target profile #89316

Closed

bogner approved these changes Apr 18, 2024

View reviewed changes

bharadwajy merged commit a2face4 into llvm:main Apr 18, 2024

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

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

Uh oh!

Conversation

bharadwajy commented Apr 17, 2024

Uh oh!

github-actions bot commented Apr 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Uh oh!

damyanp commented Apr 17, 2024

Uh oh!

bharadwajy commented Apr 17, 2024

Uh oh!

bogner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bharadwajy commented Apr 17, 2024

Uh oh!

damyanp commented Apr 17, 2024

Uh oh!

damyanp commented Apr 17, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bharadwajy commented Apr 17, 2024

Uh oh!

bharadwajy commented Apr 18, 2024

Uh oh!

damyanp commented Apr 18, 2024

Uh oh!

llvm-beanz commented Apr 18, 2024

Uh oh!

damyanp commented Apr 18, 2024

Uh oh!

damyanp commented Apr 18, 2024

Uh oh!

llvm-beanz commented Apr 18, 2024

Uh oh!

llvm-beanz left a comment

Choose a reason for hiding this comment

Uh oh!

damyanp left a comment

Choose a reason for hiding this comment

Uh oh!

bharadwajy commented Apr 18, 2024

Uh oh!

Uh oh!

github-actions bot commented Apr 17, 2024 •

edited

Loading