[DirectX] Fix DXIL part header version encoding #91506

python3kgae · 2024-05-08T17:16:05Z

Move MinorVersion be the lower 8 bit.
Set DXIL version in DXContainerObjectWriter::writeObject.

Move MinorVersion be the lower 8 bit. Set DXIL version in DXContainerObjectWriter::writeObject. Also always normalize target triple in llc to get DXIL version from subArch. Fixes llvm#89952

llvmbot · 2024-05-08T17:16:26Z

@llvm/pr-subscribers-mc
@llvm/pr-subscribers-llvm-binary-utilities

@llvm/pr-subscribers-backend-directx

Author: Xiang Li (python3kgae)

Changes

Move MinorVersion be the lower 8 bit.
Set DXIL version in DXContainerObjectWriter::writeObject.

Also always normalize target triple in llc to get DXIL version from subArch.

Fixes #89952

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

5 Files Affected:

(modified) llvm/include/llvm/BinaryFormat/DXContainer.h (+1-1)
(modified) llvm/lib/MC/MCDXContainerWriter.cpp (+3)
(modified) llvm/test/CodeGen/DirectX/embed-dxil.ll (+2-2)
(modified) llvm/tools/llc/llc.cpp (+2-2)
(modified) llvm/unittests/Object/DXContainerTest.cpp (+51-6)

diff --git a/llvm/include/llvm/BinaryFormat/DXContainer.h b/llvm/include/llvm/BinaryFormat/DXContainer.h
index e8d03f806715f..e5fcda63910d4 100644
--- a/llvm/include/llvm/BinaryFormat/DXContainer.h
+++ b/llvm/include/llvm/BinaryFormat/DXContainer.h
@@ -103,8 +103,8 @@ struct PartHeader {
 
 struct BitcodeHeader {
   uint8_t Magic[4];     // ACSII "DXIL".
-  uint8_t MajorVersion; // DXIL version.
   uint8_t MinorVersion; // DXIL version.
+  uint8_t MajorVersion; // DXIL version.
   uint16_t Unused;
   uint32_t Offset; // Offset to LLVM bitcode (from start of header).
   uint32_t Size;   // Size of LLVM bitcode (in bytes).
diff --git a/llvm/lib/MC/MCDXContainerWriter.cpp b/llvm/lib/MC/MCDXContainerWriter.cpp
index 0580dc7e42826..015899278f379 100644
--- a/llvm/lib/MC/MCDXContainerWriter.cpp
+++ b/llvm/lib/MC/MCDXContainerWriter.cpp
@@ -127,6 +127,9 @@ uint64_t DXContainerObjectWriter::writeObject(MCAssembler &Asm,
       // The program header's size field is in 32-bit words.
       Header.Size = (SectionSize + sizeof(dxbc::ProgramHeader) + 3) / 4;
       memcpy(Header.Bitcode.Magic, "DXIL", 4);
+      VersionTuple DXILVersion = TT.getDXILVersion();
+      Header.Bitcode.MajorVersion = DXILVersion.getMajor();
+      Header.Bitcode.MinorVersion = DXILVersion.getMinor().value_or(0);
       Header.Bitcode.Offset = sizeof(dxbc::BitcodeHeader);
       Header.Bitcode.Size = SectionSize;
       if (sys::IsBigEndianHost)
diff --git a/llvm/test/CodeGen/DirectX/embed-dxil.ll b/llvm/test/CodeGen/DirectX/embed-dxil.ll
index 306e5c385b5a3..9f4fb19d86fa0 100644
--- a/llvm/test/CodeGen/DirectX/embed-dxil.ll
+++ b/llvm/test/CodeGen/DirectX/embed-dxil.ll
@@ -42,8 +42,8 @@ define i32 @add(i32 %a, i32 %b) {
 ; DXC-NEXT:       MinorVersion:    5
 ; DXC-NEXT:       ShaderKind:      6
 ; DXC-NEXT:       Size:            [[#div(SIZE,4)]]
-; DXC-NEXT:       DXILMajorVersion: [[#]]
-; DXC-NEXT:       DXILMinorVersion: [[#]]
+; DXC-NEXT:       DXILMajorVersion: 1
+; DXC-NEXT:       DXILMinorVersion: 5
 ; DXC-NEXT:       DXILSize:        [[#SIZE - 24]]
 ; DXC-NEXT:       DXIL:            [ 0x42, 0x43, 0xC0, 0xDE,
 ; DXC:      - Name:            SFI0
diff --git a/llvm/tools/llc/llc.cpp b/llvm/tools/llc/llc.cpp
index b292f70ba89de..5825e79156774 100644
--- a/llvm/tools/llc/llc.cpp
+++ b/llvm/tools/llc/llc.cpp
@@ -537,8 +537,8 @@ static int compileModule(char **argv, LLVMContext &Context) {
       // If we are supposed to override the target triple, do so now.
       std::string IRTargetTriple = DataLayoutTargetTriple.str();
       if (!TargetTriple.empty())
-        IRTargetTriple = Triple::normalize(TargetTriple);
-      TheTriple = Triple(IRTargetTriple);
+        IRTargetTriple = TargetTriple;
+      TheTriple = Triple(Triple::normalize(IRTargetTriple));
       if (TheTriple.getTriple().empty())
         TheTriple.setTriple(sys::getDefaultTargetTriple());
 
diff --git a/llvm/unittests/Object/DXContainerTest.cpp b/llvm/unittests/Object/DXContainerTest.cpp
index da640225617d4..28df84578113d 100644
--- a/llvm/unittests/Object/DXContainerTest.cpp
+++ b/llvm/unittests/Object/DXContainerTest.cpp
@@ -126,6 +126,51 @@ TEST(DXCFile, ParseOverlappingParts) {
           "Part offset for part 1 begins before the previous part ends"));
 }
 
+// This test verify DXILMajorVersion and DXILMinorVersion are correctly parsed.
+// This test is based on the binary output constructed from this yaml.
+// --- !dxcontainer
+// Header:
+//   Hash:            [ 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 
+//                      0x0, 0x0, 0x0, 0x0, 0x0, 0x0 ]
+//   Version:
+//     Major:           1
+//     Minor:           0
+//   PartCount:       1
+// Parts:
+//   - Name:            DXIL
+//     Size:            28
+//     Program:
+//       MajorVersion:    6
+//       MinorVersion:    5
+//       ShaderKind:      5
+//       Size:            8
+//       DXILMajorVersion: 1
+//       DXILMinorVersion: 5
+//       DXILSize:        4
+//       DXIL:            [ 0x42, 0x43, 0xC0, 0xDE, ]
+// ...
+TEST(DXCFile, ParseDXILPart) {
+  uint8_t Buffer[] = {
+      0x44, 0x58, 0x42, 0x43, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
+      0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00,
+      0x48, 0x00, 0x00, 0x00, 0x01, 0x00, 0x00, 0x00, 0x24, 0x00, 0x00, 0x00,
+      0x44, 0x58, 0x49, 0x4c, 0x1c, 0x00, 0x00, 0x00, 0x65, 0x00, 0x05, 0x00,
+      0x08, 0x00, 0x00, 0x00, 0x44, 0x58, 0x49, 0x4c, 0x05, 0x01, 0x00, 0x00,
+      0x10, 0x00, 0x00, 0x00, 0x04, 0x00, 0x00, 0x00, 0x42, 0x43, 0xc0, 0xde};
+  DXContainer C =
+      llvm::cantFail(DXContainer::create(getMemoryBuffer<116>(Buffer)));
+  EXPECT_EQ(C.getHeader().PartCount, 1u);
+  const std::optional<object::DXContainer::DXILData> &DXIL = C.getDXIL();
+  EXPECT_TRUE(DXIL.has_value());
+  dxbc::ProgramHeader Header = DXIL->first;
+  EXPECT_EQ(Header.MajorVersion, 6u);
+  EXPECT_EQ(Header.MinorVersion, 5u);
+  EXPECT_EQ(Header.ShaderKind, 5u);
+  EXPECT_EQ(Header.Size, 8u);
+  EXPECT_EQ(Header.Bitcode.MajorVersion, 1u);
+  EXPECT_EQ(Header.Bitcode.MinorVersion, 5u);
+}
+
 TEST(DXCFile, ParseEmptyParts) {
   uint8_t Buffer[] = {
       0x44, 0x58, 0x42, 0x43, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
@@ -240,8 +285,8 @@ TEST(DXCFile, PSVResourceIterators) {
       MinorVersion:    0
       ShaderKind:      14
       Size:            6
-      DXILMajorVersion: 0
-      DXILMinorVersion: 1
+      DXILMajorVersion: 1
+      DXILMinorVersion: 0
       DXILSize:        0
 ...
 )";
@@ -361,8 +406,8 @@ TEST(DXCFile, PSVResourceIterators) {
 //       MinorVersion:    0
 //       ShaderKind:      14
 //       Size:            6
-//       DXILMajorVersion: 0
-//       DXILMinorVersion: 1
+//       DXILMajorVersion: 1
+//       DXILMinorVersion: 0
 //       DXILSize:        0
 //   - Name:            PSV0
 //     Size:            36
@@ -491,8 +536,8 @@ TEST(DXCFile, MaliciousFiles) {
 //       MinorVersion:    0
 //       ShaderKind:      14
 //       Size:            6
-//       DXILMajorVersion: 0
-//       DXILMinorVersion: 1
+//       DXILMajorVersion: 1
+//       DXILMinorVersion: 0
 //       DXILSize:        0
 //   - Name:            PSV0
 //     Size:            100

github-actions · 2024-05-08T17:19:03Z

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

bharadwajy

I see failures of LLVM regression tests (target check-llvm) with these changes.

Total Discovered Tests: 59475
Skipped : 346 (0.58%)
Unsupported : 27503 (46.24%)
Passed : 31174 (52.42%)
Expectedly Failed: 63 (0.11%)
Failed : 389 (0.65%)
FAILED: test/CMakeFiles/check-llvm /workspace/github/oss/llvm-build/hlsl/Debug/test/CMakeFiles/check-llvm

No such failures without this change (i.e., commit set to 8d9b154)

Total Discovered Tests: 59474
Skipped : 346 (0.58%)
Unsupported : 27503 (46.24%)
Passed : 31562 (53.07%)
Expectedly Failed: 63 (0.11%)

Failures appear to be related to changes in llvm/tools/llc/llc.cpp

Please double check. Thanks!

python3kgae · 2024-05-08T20:36:01Z

I see failures of LLVM regression tests (target check-llvm) with these changes.

Total Discovered Tests: 59475 Skipped : 346 (0.58%) Unsupported : 27503 (46.24%) Passed : 31174 (52.42%) Expectedly Failed: 63 (0.11%) Failed : 389 (0.65%) FAILED: test/CMakeFiles/check-llvm /workspace/github/oss/llvm-build/hlsl/Debug/test/CMakeFiles/check-llvm

No such failures without this change (i.e., commit set to 8d9b154)

Total Discovered Tests: 59474 Skipped : 346 (0.58%) Unsupported : 27503 (46.24%) Passed : 31562 (53.07%) Expectedly Failed: 63 (0.11%)

Failures appear to be related to changes in llvm/tools/llc/llc.cpp

Please double check. Thanks!

Thanks. Looking at it.

bogner · 2024-05-09T18:54:50Z

llvm/tools/llc/llc.cpp

-        IRTargetTriple = Triple::normalize(TargetTriple);
+        IRTargetTriple = TargetTriple;
+      if (!IRTargetTriple.empty())
+        IRTargetTriple = Triple::normalize(IRTargetTriple);


I don't think we should be changing llc to choose to always normalize triples buried in a DirectX change like this. This is a pretty significant change to the behaviour of llc when a module has an non-normalized triple, and if we really want to normalize the triple unconditionally I think that's a bigger change that needs wider discussion. Note that the Triple class deliberately does not normalize on construction, so that non-normalized triples can be worked with when appropriate (such as if it's intentionally broken for testing purposes, or something along those lines)

bharadwajy · 2024-05-09T20:15:44Z

llvm/tools/llc/llc.cpp

-        IRTargetTriple = TargetTriple;
-      if (!IRTargetTriple.empty())
-        IRTargetTriple = Triple::normalize(IRTargetTriple);
+        IRTargetTriple = Triple::normalize(TargetTriple);


Is this change still needed? Is it possible to avoid making changes to this file?

No. There's no change in llc.cpp now.

bharadwajy

Just a reminder to make sure the sentence

"Also always normalize target triple in llc to get DXIL version from subArch."

is edited out of the "Squash and Merge" commit message.

bogner · 2024-05-09T22:46:21Z

llvm/lib/TargetParser/Triple.cpp

@@ -1510,6 +1510,8 @@ VersionTuple Triple::getDXILVersion() const {
  if (getArch() != dxil || getOS() != ShaderModel)
    llvm_unreachable("invalid DXIL triple");
  StringRef Arch = getArchName();
+  if (Arch == "dxil")


I think you can say getSubArch() == NoSubArch here instead of using string comparison

python3kgae · 2024-05-10T02:24:27Z

Just a reminder to make sure the sentence

"Also always normalize target triple in llc to get DXIL version from subArch."

is edited out of the "Squash and Merge" commit message.
Good catch.
Updated.

This reverts commit 195d8ac.

Test failures on big endian bots after this change. Reverts #91506

[DirectX] Fix DXIL part header version encoding

9cd6ecf

Move MinorVersion be the lower 8 bit. Set DXIL version in DXContainerObjectWriter::writeObject. Also always normalize target triple in llc to get DXIL version from subArch. Fixes llvm#89952

python3kgae added the backend:DirectX label May 8, 2024

llvmbot added mc Machine (object) code llvm:binary-utilities labels May 8, 2024

Fix clang-format

c04c073

llvm-beanz approved these changes May 8, 2024

View reviewed changes

damyanp approved these changes May 8, 2024

View reviewed changes

bharadwajy reviewed May 8, 2024

View reviewed changes

Only normlize Triple when not empty.

ade02d7

python3kgae force-pushed the dx_ver_order branch from 0d62735 to ade02d7 Compare May 8, 2024 20:49

bogner reviewed May 9, 2024

View reviewed changes

getDXILVersion from OS when there's no SubArch but "dxil" only.

b852b81

python3kgae force-pushed the dx_ver_order branch from d5514d3 to b852b81 Compare May 9, 2024 19:04

bharadwajy reviewed May 9, 2024

View reviewed changes

bharadwajy approved these changes May 9, 2024

View reviewed changes

bogner approved these changes May 9, 2024

View reviewed changes

Avoid string check.

2802752

python3kgae merged commit 195d8ac into llvm:main May 10, 2024

python3kgae deleted the dx_ver_order branch May 10, 2024 13:29

bogner added a commit that referenced this pull request May 10, 2024

Revert "[DirectX] Fix DXIL part header version encoding (#91506)"

f5cec3a

This reverts commit 195d8ac.

bogner mentioned this pull request May 10, 2024

Revert "[DirectX] Fix DXIL part header version encoding" #91791

Merged

bogner added a commit that referenced this pull request May 10, 2024

Revert "[DirectX] Fix DXIL part header version encoding" (#91791)

d655054

Test failures on big endian bots after this change. Reverts #91506

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[DirectX] Fix DXIL part header version encoding #91506

[DirectX] Fix DXIL part header version encoding #91506

Uh oh!

python3kgae commented May 8, 2024 •

edited

Loading

Uh oh!

llvmbot commented May 8, 2024 •

edited

Loading

Uh oh!

github-actions bot commented May 8, 2024 •

edited

Loading

Uh oh!

bharadwajy left a comment

Uh oh!

python3kgae commented May 8, 2024

Uh oh!

bogner May 9, 2024

Uh oh!

python3kgae May 9, 2024

Uh oh!

bharadwajy May 9, 2024

Uh oh!

python3kgae May 9, 2024

Uh oh!

bharadwajy left a comment

Uh oh!

bogner May 9, 2024

Uh oh!

python3kgae May 9, 2024

Uh oh!

python3kgae commented May 10, 2024

Uh oh!

Uh oh!

[DirectX] Fix DXIL part header version encoding #91506

[DirectX] Fix DXIL part header version encoding #91506

Uh oh!

Conversation

python3kgae commented May 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

llvmbot commented May 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

bharadwajy left a comment

Choose a reason for hiding this comment

Uh oh!

python3kgae commented May 8, 2024

Uh oh!

bogner May 9, 2024

Choose a reason for hiding this comment

Uh oh!

python3kgae May 9, 2024

Choose a reason for hiding this comment

Uh oh!

bharadwajy May 9, 2024

Choose a reason for hiding this comment

Uh oh!

python3kgae May 9, 2024

Choose a reason for hiding this comment

Uh oh!

bharadwajy left a comment

Choose a reason for hiding this comment

Uh oh!

bogner May 9, 2024

Choose a reason for hiding this comment

Uh oh!

python3kgae May 9, 2024

Choose a reason for hiding this comment

Uh oh!

python3kgae commented May 10, 2024

Uh oh!

Uh oh!

python3kgae commented May 8, 2024 •

edited

Loading

llvmbot commented May 8, 2024 •

edited

Loading

github-actions bot commented May 8, 2024 •

edited

Loading