Skip to content

[DirectX] Remove unused global variables #149184

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.

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

Closed
wants to merge 2 commits into from

Conversation

kmpeng
Copy link
Contributor

@kmpeng kmpeng commented Jul 16, 2025

Fixes #139023

This PR:

  • Removes unused global variables in the finalize linkage pass
  • Changes the scalar-data.ll run command to avoid removing its global variables
  • Adds tests to finalize_linkage.ll that make sure unused global variables are removed
  • Temporarily removes the llc run command from finalize_linkage.ll

@kmpeng kmpeng marked this pull request as ready for review July 16, 2025 20:52
@llvmbot
Copy link
Member

llvmbot commented Jul 16, 2025

@llvm/pr-subscribers-backend-directx

Author: Kaitlin Peng (kmpeng)

Changes

Fixes #139023.

This PR:

  • Removes unused global variables in the finalize linkage pass
  • Changes the scalar-data.ll run command to avoid removing its global variables
  • Temporarily removes the llc run command from finalize_linkage.ll

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

3 Files Affected:

  • (modified) llvm/lib/Target/DirectX/DXILFinalizeLinkage.cpp (+12)
  • (modified) llvm/test/CodeGen/DirectX/finalize_linkage.ll (+20-2)
  • (modified) llvm/test/CodeGen/DirectX/scalar-data.ll (+1-1)
diff --git a/llvm/lib/Target/DirectX/DXILFinalizeLinkage.cpp b/llvm/lib/Target/DirectX/DXILFinalizeLinkage.cpp
index 5f331dbd2d826..cf497d8e4bba5 100644
--- a/llvm/lib/Target/DirectX/DXILFinalizeLinkage.cpp
+++ b/llvm/lib/Target/DirectX/DXILFinalizeLinkage.cpp
@@ -28,6 +28,18 @@ static bool finalizeLinkage(Module &M) {
     }
   }
 
+  // Remove unused global variables.
+  SmallVector<GlobalVariable *> ToErase;
+  for (GlobalVariable &GV : M.globals()) {
+    if (GV.use_empty()) {
+      ToErase.push_back(&GV);
+    }
+  }
+  for (GlobalVariable *GV : ToErase) {
+    GV->eraseFromParent();
+    MadeChange = true;
+  }
+
   SmallVector<Function *> Funcs;
 
   // Collect non-entry and non-exported functions to set to internal linkage.
diff --git a/llvm/test/CodeGen/DirectX/finalize_linkage.ll b/llvm/test/CodeGen/DirectX/finalize_linkage.ll
index dc1140f1c9160..b0e9182c813f7 100644
--- a/llvm/test/CodeGen/DirectX/finalize_linkage.ll
+++ b/llvm/test/CodeGen/DirectX/finalize_linkage.ll
@@ -1,10 +1,17 @@
 ; RUN: opt -S -dxil-finalize-linkage -mtriple=dxil-unknown-shadermodel6.5-compute %s | FileCheck %s
-; RUN: llc %s --filetype=asm -o - | FileCheck %s --check-prefixes=CHECK-LLC
+; TODO: Add back the llc test once #149179 and #149180 are fixed
 
 target triple = "dxilv1.5-pc-shadermodel6.5-compute"
 
 ; DXILFinalizeLinkage changes linkage of all functions that are hidden to
-; internal, and converts private global variables to internal linkage.
+; internal, converts private global variables to internal linkage, and removes
+; unused global variables.
+
+; CHECK-NOT: @aTile
+@aTile = hidden addrspace(3) global [4 x [1 x i32]] zeroinitializer, align 4
+
+; CHECK-NOT: @bTile
+@bTile = hidden addrspace(3) global [1 x <1 x i32>] zeroinitializer, align 4
 
 ; CHECK: @switch.table = internal unnamed_addr constant [4 x i32]
 @switch.table = private unnamed_addr constant [4 x i32] [i32 1, i32 257, i32 65793, i32 16843009], align 4
@@ -27,6 +34,17 @@ target triple = "dxilv1.5-pc-shadermodel6.5-compute"
 ; CHECK: @hidden_var = hidden global i32
 @hidden_var = hidden global i32 1, align 4
 
+define void @anchor_function() #0 {
+entry:
+  %0 = load i32, ptr @switch.table, align 4
+  %1 = load [3 x float], ptr @private_array, align 4
+  %2 = load i32, ptr @private_var, align 4
+  %3 = load i32, ptr @internal_var, align 4
+  %4 = load i32, ptr @external_var, align 4
+  %5 = load i32, ptr @hidden_var, align 4
+  ret void
+}
+
 ; CHECK-NOT: define internal void @"?f1@@YAXXZ"()
 define void @"?f1@@YAXXZ"() #0 {
 entry:
diff --git a/llvm/test/CodeGen/DirectX/scalar-data.ll b/llvm/test/CodeGen/DirectX/scalar-data.ll
index 4861a0890f136..d9c8df9bc169c 100644
--- a/llvm/test/CodeGen/DirectX/scalar-data.ll
+++ b/llvm/test/CodeGen/DirectX/scalar-data.ll
@@ -1,4 +1,4 @@
-; RUN: llc %s -mtriple=dxil-pc-shadermodel6.3-library --filetype=asm -o - | FileCheck %s
+; RUN: opt -S -passes='dxil-data-scalarization,dxil-flatten-arrays' -mtriple=dxil-unknown-shadermodel6.5-compute %s | FileCheck %s
 
 ; Make sure we don't touch arrays without vectors and that can recurse and flatten multiple-dimension arrays of vectors
 

SmallVector<GlobalVariable *> ToErase;
for (GlobalVariable &GV : M.globals()) {
if (GV.use_empty()) {
ToErase.push_back(&GV);
Copy link
Member

Choose a reason for hiding this comment

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

any chance we could just do GV->eraseFromParent(); here and not need the second loop\ ToErase vector?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, that causes it to crash. Doing GV->eraseFromParent() here removes it from the module's global list, which invalidates the iterator used in the for loop

Copy link
Member

@farzonl farzonl Jul 16, 2025

Choose a reason for hiding this comment

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

I'm pretty sure you can get around that problem with make_early_inc_range. To be clear what I'm asking. Is it possible to do

for (GlobalVariable &GV : llvm::make_early_inc_range(M.globals())) {
  GV.eraseFromParent();
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah that works, I just did it this way because I was trying to follow the same format as the function removals. But I can change it

@kmpeng kmpeng force-pushed the bugfix/unused-globalvars branch from 6b8ee3b to eddc1cf Compare July 16, 2025 21:28
Comment on lines +32 to +37
for (GlobalVariable &GV : make_early_inc_range(M.globals())) {
if (GV.use_empty()) {
GV.eraseFromParent();
MadeChange = true;
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

style nit: unecessary curly braces

Suggested change
for (GlobalVariable &GV : make_early_inc_range(M.globals())) {
if (GV.use_empty()) {
GV.eraseFromParent();
MadeChange = true;
}
}
for (GlobalVariable &GV : make_early_inc_range(M.globals()))
if (GV.use_empty()) {
GV.eraseFromParent();
MadeChange = true;
}

@@ -1,10 +1,17 @@
; RUN: opt -S -dxil-finalize-linkage -mtriple=dxil-unknown-shadermodel6.5-compute %s | FileCheck %s
; RUN: llc %s --filetype=asm -o - | FileCheck %s --check-prefixes=CHECK-LLC
; TODO: Add back the llc test once #149179 and #149180 are fixed
Copy link
Contributor

Choose a reason for hiding this comment

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

These are fixed now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, thanks!

@Icohedron
Copy link
Contributor

Icohedron commented Jul 17, 2025

I recompiled all the DML shaders with this PR and it appears to completely resolve #139023.
I did not include changes from your other PR #148986 when building, so PR #148986 is not necessary to fix #139023.
However, that PR is still required for correctness.

Copy link
Contributor

@Icohedron Icohedron left a comment

Choose a reason for hiding this comment

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

The PR changes look good but you need to add the llc test since #149179 and #149180 are fixed.

Furthermore, you need to fix these dxil-dis tests which have broken with your changes:

Failed Tests (2):
  LLVM :: tools/dxil-dis/global_ctor.ll
  LLVM :: tools/dxil-dis/opaque-value_as_metadata.ll

If you have configured llvm with -DLLVM_INCLUDE_DXIL_TESTS=ON, you can run these tests by building the target check-llvm-tools-dxil-dis

@kmpeng
Copy link
Contributor Author

kmpeng commented Jul 17, 2025

I recompiled all the DML shaders with this PR and it appears to completely resolve #139023. I did not include changes from your other PR #148986 when building, so PR #148986 is not necessary to fix #139023. However, that PR is still required for correctness.

Yep, we realized that but didn't update it in the comments sorry. I think Farzon said he wants to redo the solution to #148986 to make it recursive, so I'll be closing that PR soon.

@kmpeng
Copy link
Contributor Author

kmpeng commented Jul 29, 2025

New PR that adds the GlobalDCE pass after finalize linkage instead of this approach: #151071

@kmpeng kmpeng closed this Jul 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DirectX] Validator is erroring with Internal declaration '.*' is unused
4 participants