Skip to content

[DirectX] Limit GEP transformations to Arrays #145758

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

Merged
merged 1 commit into from
Jun 25, 2025

Conversation

farzonl
Copy link
Member

@farzonl farzonl commented Jun 25, 2025

fixes #145408

Before we see the GEP we already have transformed Allocas that get passed the isArrayOfVectors.
The bug is because we are trying to transform a gep for struct of arrays when we should only be transforming arrays.

The problem with our visitGetElementPtrInst is that it was doing transformations for all allocas when it should be limiting it to the alloca of array cases. Technically we would have liked to make sure it was an array of vectors cases but by the time we see the GEP the type has been changed by replace all uses. There should not be a problem with looking at all Arrays since DXILDataScalarization does not change any indicies.

fixes llvm#145408

Before we see the GEP we already have transformed Allocas that
get passed the `isArrayOfVectors`.
The problem with our `visitGetElementPtrInst` is that it was
doing transformations for all allocas when it should be limiting it
to the array cases. Technically we would have liked to make
sure it was an array of vectors cases but by the time we see
the GEP the type has been changed by replace all uses.
There should not be a problem with looking at all Arrays
since DXILDataScalarization does not change any indicies.
@llvmbot
Copy link
Member

llvmbot commented Jun 25, 2025

@llvm/pr-subscribers-backend-directx

Author: Farzon Lotfi (farzonl)

Changes

fixes #145408

Before we see the GEP we already have transformed Allocas that get passed the isArrayOfVectors.
The bug is because we are trying to transform a gep for struct of arrays when we should only be transforming arrays.

The problem with our visitGetElementPtrInst is that it was doing transformations for all allocas when it should be limiting it to the alloca of array cases. Technically we would have liked to make sure it was an array of vectors cases but by the time we see the GEP the type has been changed by replace all uses. There should not be a problem with looking at all Arrays since DXILDataScalarization does not change any indicies.


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

2 Files Affected:

  • (modified) llvm/lib/Target/DirectX/DXILDataScalarization.cpp (+2-3)
  • (added) llvm/test/CodeGen/DirectX/issue-145408-gep-struct-fix.ll (+17)
diff --git a/llvm/lib/Target/DirectX/DXILDataScalarization.cpp b/llvm/lib/Target/DirectX/DXILDataScalarization.cpp
index 0a9b2bb99f7eb..71eb1349314ea 100644
--- a/llvm/lib/Target/DirectX/DXILDataScalarization.cpp
+++ b/llvm/lib/Target/DirectX/DXILDataScalarization.cpp
@@ -308,9 +308,8 @@ bool DataScalarizerVisitor::visitGetElementPtrInst(GetElementPtrInst &GEPI) {
     NeedsTransform = true;
   } else if (AllocaInst *Alloca = dyn_cast<AllocaInst>(PtrOperand)) {
     Type *AllocatedType = Alloca->getAllocatedType();
-    // OrigGEPType might just be a pointer lets make sure
-    // to add the allocated type so we have a size
-    if (AllocatedType != OrigGEPType) {
+    // Only transform if the allocated type is an array
+    if (AllocatedType != OrigGEPType && isa<ArrayType>(AllocatedType)) {
       NewGEPType = AllocatedType;
       NeedsTransform = true;
     }
diff --git a/llvm/test/CodeGen/DirectX/issue-145408-gep-struct-fix.ll b/llvm/test/CodeGen/DirectX/issue-145408-gep-struct-fix.ll
new file mode 100644
index 0000000000000..40d222cdf2f8f
--- /dev/null
+++ b/llvm/test/CodeGen/DirectX/issue-145408-gep-struct-fix.ll
@@ -0,0 +1,17 @@
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 5
+; RUN: opt -S -dxil-data-scalarization -mtriple=dxil-pc-shadermodel6.3-library %s | FileCheck %s
+
+%struct.RawStruct8D = type { [8 x i32] }
+
+define void @test_no_transform_of_struct()  {
+; CHECK-LABEL: define void @test_no_transform_of_struct() {
+; CHECK-NEXT:  [[ENTRY:.*:]]
+; CHECK-NEXT:    [[OUTPUTSIZESLOCAL_I:%.*]] = alloca [[STRUCT_RAWSTRUCT8D:%.*]], align 4
+; CHECK-NEXT:    [[ARRAYINIT_ELEMENT13_I76:%.*]] = getelementptr inbounds nuw [1 x %struct.RawStruct8D], ptr [[OUTPUTSIZESLOCAL_I]], i32 0, i32 0
+; CHECK-NEXT:    ret void
+;
+entry:
+  %outputSizesLocal.i = alloca %struct.RawStruct8D, align 4
+  %arrayinit.element13.i76 = getelementptr inbounds nuw [1 x %struct.RawStruct8D], ptr %outputSizesLocal.i, i32 0, i32 0
+  ret void
+}

// to add the allocated type so we have a size
if (AllocatedType != OrigGEPType) {
// Only transform if the allocated type is an array
if (AllocatedType != OrigGEPType && isa<ArrayType>(AllocatedType)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we should be more restrictive here to be consistent with the visitAllocaInst:

if (!isArrayOfVectors(AI.getAllocatedType()))
.

In general, this pass only seems interested to transform arrays of vectors. so we shouldn't need to transform a simple array of eg i32. Can we extend the testcase to demonstrate this scenario as well?

Copy link
Member Author

@farzonl farzonl Jun 25, 2025

Choose a reason for hiding this comment

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

We can’t. See the description. The type has already changed by the time we see the gep.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right, but IIUC, then it should just be an array of arrays now that we could check against?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was going to say array is correct because vectors should be converted to arrays but it seems we only have test for that when its a global not an alloca. So I think isArrayOfVectors needs to be isVectorOrArrayOfVectors we just haven't encountered a bug for this yet. I think we need to file a new bug.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just to keep this change more focused, I'm going to address the bug in the alloca visitor sepreately: #145782

Copy link
Contributor

@inbelic inbelic left a comment

Choose a reason for hiding this comment

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

LGTM, given the other issue is tracked.

@farzonl farzonl merged commit 38aec4f into llvm:main Jun 25, 2025
10 checks passed
anthonyhatran pushed a commit to anthonyhatran/llvm-project that referenced this pull request Jun 26, 2025
fixes llvm#145408

Before we see the GEP we already have transformed Allocas that get
passed the `isArrayOfVectors`.
The bug is because we are trying to transform a gep for struct of arrays
when we should only be transforming arrays.

The problem with our `visitGetElementPtrInst` is that it was doing
transformations for all allocas when it should be limiting it to the
alloca of array cases. Technically we would have liked to make sure it
was an array of vectors cases but by the time we see the GEP the type
has been changed by replace all uses. There should not be a problem with
looking at all Arrays since DXILDataScalarization does not change any
indicies.
@farzonl farzonl self-assigned this Jun 27, 2025
rlavaee pushed a commit to rlavaee/llvm-project that referenced this pull request Jul 1, 2025
fixes llvm#145408

Before we see the GEP we already have transformed Allocas that get
passed the `isArrayOfVectors`.
The bug is because we are trying to transform a gep for struct of arrays
when we should only be transforming arrays.

The problem with our `visitGetElementPtrInst` is that it was doing
transformations for all allocas when it should be limiting it to the
alloca of array cases. Technically we would have liked to make sure it
was an array of vectors cases but by the time we see the GEP the type
has been changed by replace all uses. There should not be a problem with
looking at all Arrays since DXILDataScalarization does not change any
indicies.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

[DirectX] Segfault in PointerTypeAnalysis.cpp classifyPointerType
4 participants