Adjust bit cast instruction filter for DXIL Prepare pass #142678

bob80905 · 2025-06-03T22:05:33Z

This PR addresses a specific edge case when deciding whether or not to produce a bitcast instruction.
Specifically, when the given instruction is a global array, the element type of the array wasn't correctly compared to the return type. In this specific case, if the types are equal, a bitcast shouldn't be created, but it was.
This PR checks to see if the element type of the array is the same as the return type, and if it is, it doesn't create a bitcast instruction.

Fixes #139013

llvmbot · 2025-06-03T22:06:08Z

@llvm/pr-subscribers-backend-directx

Author: Joshua Batista (bob80905)

Changes

This PR addresses a specific edge case when deciding whether or not to produce a bitcast instruction.
Specifically, when the given instruction is a global array, the element type of the array wasn't correctly compared to the return type. In this specific case, if the types are equal, a bitcast shouldn't be created, but it was.
This PR checks to see if the element type of the array is the same as the return type, and if it is, it doesn't create a bitcast instruction.

Fixes #139013

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

2 Files Affected:

(modified) llvm/lib/Target/DirectX/DXILPrepare.cpp (+35-2)
(added) llvm/test/CodeGen/DirectX/noop_bitcast_global_array_type.ll (+39)

diff --git a/llvm/lib/Target/DirectX/DXILPrepare.cpp b/llvm/lib/Target/DirectX/DXILPrepare.cpp
index e0068787f5e5a..54e4499830ff0 100644
--- a/llvm/lib/Target/DirectX/DXILPrepare.cpp
+++ b/llvm/lib/Target/DirectX/DXILPrepare.cpp
@@ -148,9 +148,42 @@ class DXILPrepareModule : public ModulePass {
                                      Type *Ty) {
     // Omit bitcasts if the incoming value matches the instruction type.
     auto It = PointerTypes.find(Operand);
-    if (It != PointerTypes.end())
-      if (cast<TypedPointerType>(It->second)->getElementType() == Ty)
+    if (It != PointerTypes.end()) {
+      auto OpTy = cast<TypedPointerType>(It->second)->getElementType();
+      if (OpTy == Ty)
         return nullptr;
+    }
+
+    // Also omit the bitcast for matching global array types
+    if (auto *GlobalVar = llvm::dyn_cast<llvm::GlobalVariable>(Operand)) {
+      llvm::Type *ValTy = GlobalVar->getValueType();
+
+      if (auto *ArrTy = llvm::dyn_cast<llvm::ArrayType>(ValTy)) {
+        llvm::Type *ElTy = ArrTy->getElementType();
+        if (ElTy == Ty)
+          return nullptr;
+      }
+    }
+
+    // finally, drill down GEP instructions until we get the array
+    // that is being accessed, and compare element types
+    if (auto *GEPInstr = llvm::dyn_cast<llvm::ConstantExpr>(Operand)) {
+      while (GEPInstr->getOpcode() == llvm::Instruction::GetElementPtr) {
+        llvm::Value *OpArg = GEPInstr->getOperand(0);
+
+        if (auto *GlobalVar = llvm::dyn_cast<llvm::GlobalVariable>(OpArg)) {
+          llvm::Constant *initializer = GlobalVar->getInitializer();
+
+          llvm::Type *ValTy = GlobalVar->getValueType();
+          if (auto *ArrTy = llvm::dyn_cast<llvm::ArrayType>(ValTy)) {
+            llvm::Type *ElTy = ArrTy->getElementType();
+            if (ElTy == Ty)
+              return nullptr;
+          }
+        }
+      }
+    }
+
     // Insert bitcasts where we are removing the instruction.
     Builder.SetInsertPoint(&Inst);
     // This code only gets hit in opaque-pointer mode, so the type of the
diff --git a/llvm/test/CodeGen/DirectX/noop_bitcast_global_array_type.ll b/llvm/test/CodeGen/DirectX/noop_bitcast_global_array_type.ll
new file mode 100644
index 0000000000000..4cca76efcf461
--- /dev/null
+++ b/llvm/test/CodeGen/DirectX/noop_bitcast_global_array_type.ll
@@ -0,0 +1,39 @@
+; RUN: opt -S --dxil-prepare %s | FileCheck %s
+
+; Test that global arrays do not get a bitcast instruction
+; after the dxil-prepare pass.
+
+target triple = "dxilv1.2-unknown-shadermodel6.2-compute"
+
+@inputTile.1dim = local_unnamed_addr addrspace(3) global [3 x float] zeroinitializer, align 2
+
+define float @testload() local_unnamed_addr {
+  ; NOTE: this would be "bitcast ptr addrspace(3)..." before the change that introduced this test,
+  ; after the dxil-prepare pass is run
+  ; CHECK: load float, ptr addrspace(3) @inputTile.1dim, align 2
+  %v = load float, ptr addrspace(3) @inputTile.1dim, align 2  
+  
+  ret float %v
+}
+
+define void @teststore() local_unnamed_addr {  
+  ; CHECK: store float 2.000000e+00, ptr addrspace(3) @inputTile.1dim, align 2
+  store float 2.000000e+00, ptr addrspace(3) @inputTile.1dim, align 2  
+  
+  ret void
+}
+
+define float @testGEPConst() local_unnamed_addr {  
+  ; CHECK: load float, ptr addrspace(3) getelementptr (float, ptr addrspace(3) @inputTile.1dim, i32 1), align 4
+  %v = load float, ptr addrspace(3) getelementptr (float, ptr addrspace(3) @inputTile.1dim, i32 1), align 4
+  
+  ret float %v
+}
+
+define float @testGEPNonConst(i32 %i) local_unnamed_addr {  
+  ; CHECK: getelementptr float, ptr addrspace(3) @inputTile.1dim, i32 %i
+  %gep = getelementptr float, ptr addrspace(3) @inputTile.1dim, i32 %i
+  %v = load float, ptr addrspace(3) %gep
+  
+  ret float %v
+}

farzonl · 2025-06-03T23:51:02Z

llvm/test/CodeGen/DirectX/noop_bitcast_global_array_type.ll

+define float @testload() local_unnamed_addr {
+  ; NOTE: this would be "bitcast ptr addrspace(3)..." before the change that introduced this test,
+  ; after the dxil-prepare pass is run
+  ; CHECK: load float, ptr addrspace(3) @inputTile.1dim, align 2


Add check labels and since you are doing that mayve theese should all be CHECK-NEXT?

llvm/lib/Target/DirectX/DXILPrepare.cpp

farzonl · 2025-06-04T19:57:17Z

llvm/lib/Target/DirectX/DXILPrepare.cpp

+    }
+
+    // Also omit the bitcast for matching global array types
+    if (auto *GlobalVar = llvm::dyn_cast<llvm::GlobalVariable>(Operand)) {


Are we sure this is just globals? We can have alloca arrays
should we add if (auto *AI = dyn_cast<AllocaInst>(Operand))?

Yea I think we should add a case for Alloca

farzonl · 2025-06-04T19:57:58Z

llvm/lib/Target/DirectX/DXILPrepare.cpp

+    }
+
+    // Also omit the bitcast for matching global array types
+    if (auto *GlobalVar = llvm::dyn_cast<llvm::GlobalVariable>(Operand)) {


Suggested change

if (auto *GlobalVar = llvm::dyn_cast<llvm::GlobalVariable>(Operand)) {

if (auto *GlobalVar = dyn_cast<GlobalVariable>(Operand)) {

farzonl · 2025-06-04T19:58:42Z

llvm/lib/Target/DirectX/DXILPrepare.cpp

+    if (auto *GlobalVar = llvm::dyn_cast<llvm::GlobalVariable>(Operand)) {
+      llvm::Type *ValTy = GlobalVar->getValueType();
+
+      if (auto *ArrTy = llvm::dyn_cast<llvm::ArrayType>(ValTy)) {


Suggested change

if (auto *ArrTy = llvm::dyn_cast<llvm::ArrayType>(ValTy)) {

if (auto *ArrTy = dyn_cast<ArrayType>(ValTy)) {

instead of commenting on everything single one of these just remove the llvm:: from all these Types and function calls. we have using namespace llvm; defined on line 34.

farzonl

Alloca case not handled the way I was expecting.

farzonl · 2025-06-05T00:23:32Z

llvm/lib/Target/DirectX/DXILPrepare.cpp

+    }
+
+    // Also omit the bitcast for alloca instructions
+    if (auto *AI = dyn_cast<AllocaInst>(Operand)) {


no what I mean was you see how you are casting ValTy to ArrTy? So before that do

if (auto *AI = dyn_cast<AllocaInst>(Operand)) ValTy = AI->getAllocatedType();

ie

Type *ValTy = Operand->getType(); if (auto *GlobalVar = dyn_cast<GlobalVariable>(Operand)) Type *ValTy = GlobalVar->getValueType(); if (auto *AI = dyn_cast<AllocaInst>(Operand)) ValTy = AI->getAllocatedType(); if (auto *ArrTy = dyn_cast<ArrayType>(ValTy)) { Type *ElTy = ArrTy->getElementType(); if (ElTy == Ty) return nullptr; }

this is pseudo code so adjust it to something that works.

Will re-review once the Alloca handling is complete

llvm/test/CodeGen/DirectX/noop_bitcast_global_array_type.ll

…m#142678)" This reverts commit 1d6e8ec.

@b

…bit cast instruction filter for DXIL Prepare pass (#142678)" (#143043) - This reverts commit 9ab4c16. - This reverts commit 1d6e8ec. Noticed a really weird behavior where release and debug builds have different codegen for loads with geps after this PR. This is going to take a minute to debug and figure out why so revert seems to make the most sense. ```diff diff --git a/llvm/test/CodeGen/DirectX/flatten-array.ll b/llvm/test/CodeGen/DirectX/flatten-array.ll index 47d7b50..efa9efeff13a 100644 --- a/llvm/test/CodeGen/DirectX/flatten-array.ll +++ b/llvm/test/CodeGen/DirectX/flatten-array.ll @@ -123,7 +123,8 @@ define void @gep_4d_test () { @b = internal global [2 x [3 x [4 x i32]]] zeroinitializer, align 16 define void @global_gep_load() { - ; CHECK: load i32, ptr getelementptr inbounds ([24 x i32], ptr @a.1dim, i32 0, i32 6), align 4 + ; CHECK: %1 = getelementptr inbounds [24 x i32], ptr @a.1dim, i32 0, i32 6 + ; CHECK-NEXT: %2 = load i32, ptr %1, align 4 ; CHECK-NEXT: ret void %1 = getelementptr inbounds [2 x [3 x [4 x i32]]], [2 x [3 x [4 x i32]]]* @A, i32 0, i32 0 %2 = getelementptr inbounds [3 x [4 x i32]], [3 x [4 x i32]]* %1, i32 0, i32 1 @@ -176,7 +177,8 @@ define void @global_incomplete_gep_chain(i32 %row, i32 %col) { } define void @global_gep_store() { - ; CHECK: store i32 1, ptr getelementptr inbounds ([24 x i32], ptr @b.1dim, i32 0, i32 13), align 4 + ; CHECK: %1 = getelementptr inbounds [24 x i32], ptr @b.1dim, i32 0, i32 13 + ; CHECK-NEXT: store i32 1, ptr %1, align 4 ; CHECK-NEXT: ret void ```

llvmbot added the backend:DirectX label Jun 3, 2025

farzonl reviewed Jun 3, 2025

View reviewed changes

Icohedron reviewed Jun 4, 2025

View reviewed changes

llvm/lib/Target/DirectX/DXILPrepare.cpp Outdated Show resolved Hide resolved

Icohedron previously approved these changes Jun 4, 2025

View reviewed changes

farzonl reviewed Jun 4, 2025

View reviewed changes

farzonl approved these changes Jun 4, 2025

View reviewed changes

bob80905 force-pushed the remove_no_op_bitcast_DXIL branch from 2fbbbbe to 3d1ab27 Compare June 5, 2025 00:16

farzonl requested changes Jun 5, 2025

View reviewed changes

farzonl reviewed Jun 5, 2025

View reviewed changes

llvm/test/CodeGen/DirectX/noop_bitcast_global_array_type.ll Show resolved Hide resolved

farzonl self-requested a review June 5, 2025 02:04

bob80905 added 8 commits June 5, 2025 11:27

first attempt

0b6f346

fix while loop

b912dd4

add check label and check next

54a2c0b

update test

006b6df

force pointer type deduction

80d381e

remove llvm:: namespace, and add alloca case

f297a02

adjust alloca logic and test

fd6651b

fix args

b143548

bob80905 force-pushed the remove_no_op_bitcast_DXIL branch from be91577 to b143548 Compare June 5, 2025 19:14

farzonl approved these changes Jun 5, 2025

View reviewed changes

Icohedron approved these changes Jun 5, 2025

View reviewed changes

bob80905 merged commit 1d6e8ec into llvm:main Jun 5, 2025
8 of 9 checks passed

farzonl added a commit to farzonl/llvm-project that referenced this pull request Jun 6, 2025

Revert "Adjust bit cast instruction filter for DXIL Prepare pass (llv…

4ae8008

…m#142678)" This reverts commit 1d6e8ec.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Adjust bit cast instruction filter for DXIL Prepare pass #142678

Adjust bit cast instruction filter for DXIL Prepare pass #142678

Uh oh!

bob80905 commented Jun 3, 2025

Uh oh!

llvmbot commented Jun 3, 2025

Uh oh!

farzonl Jun 3, 2025

Uh oh!

Uh oh!

farzonl Jun 4, 2025 •

edited

Loading

Uh oh!

Icohedron Jun 4, 2025

Uh oh!

farzonl Jun 4, 2025

Uh oh!

farzonl Jun 4, 2025

Uh oh!

farzonl Jun 4, 2025 •

edited

Loading

Uh oh!

farzonl left a comment

Uh oh!

farzonl Jun 5, 2025 •

edited

Loading

Uh oh!

farzonl Jun 5, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

	if (auto *GlobalVar = llvm::dyn_cast<llvm::GlobalVariable>(Operand)) {
	if (auto *GlobalVar = dyn_cast<GlobalVariable>(Operand)) {

	if (auto *ArrTy = llvm::dyn_cast<llvm::ArrayType>(ValTy)) {
	if (auto *ArrTy = dyn_cast<ArrayType>(ValTy)) {

Adjust bit cast instruction filter for DXIL Prepare pass #142678

Adjust bit cast instruction filter for DXIL Prepare pass #142678

Uh oh!

Conversation

bob80905 commented Jun 3, 2025

Uh oh!

llvmbot commented Jun 3, 2025

Uh oh!

farzonl Jun 3, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

farzonl Jun 4, 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 Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

farzonl Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

farzonl Jun 4, 2025

Choose a reason for hiding this comment

Uh oh!

farzonl Jun 4, 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!

farzonl left a comment

Choose a reason for hiding this comment

Uh oh!

farzonl Jun 5, 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!

farzonl Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

farzonl Jun 4, 2025 •

edited

Loading

farzonl Jun 4, 2025 •

edited

Loading

farzonl Jun 5, 2025 •

edited

Loading