-
Notifications
You must be signed in to change notification settings - Fork 14.4k
[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
Conversation
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.
@llvm/pr-subscribers-backend-directx Author: Farzon Lotfi (farzonl) Changesfixes #145408 Before we see the GEP we already have transformed Allocas that get passed the The problem with our Full diff: https://github.com/llvm/llvm-project/pull/145758.diff 2 Files Affected:
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)) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this 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.
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.
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.
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.