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.

Sign up for GitHub

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
Merged

[DirectX] Limit GEP transformations to Arrays #145758

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions llvm/lib/Target/DirectX/DXILDataScalarization.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
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:

llvm-project/llvm/lib/Target/DirectX/DXILDataScalarization.cpp

Line 127 in 2f817d1

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 •
edited
Loading

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

NewGEPType = AllocatedType;
NeedsTransform = true;
}
Expand Down
17 changes: 17 additions & 0 deletions llvm/test/CodeGen/DirectX/issue-145408-gep-struct-fix.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
}