Skip to content

[DirectX] Reland #142853 with Circular GEP fixes #143747

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 3 commits into from
Jun 12, 2025
Merged

[DirectX] Reland #142853 with Circular GEP fixes #143747

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e0fd57c
[DirectX] Array GEPs need two indices (#142853)
farzonl Jun 5, 2025
3ebf17e
[DirectX] This change fixes a circular reference discovered in flatte…
farzonl Jun 11, 2025
55c40f9
address pr comments
farzonl Jun 12, 2025
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
24 changes: 18 additions & 6 deletions llvm/lib/Target/DirectX/DXILFlattenArrays.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ class DXILFlattenArraysLegacy : public ModulePass {

struct GEPData {
ArrayType *ParentArrayType;
Value *ParendOperand;
Value *ParentOperand;
SmallVector<Value *> Indices;
SmallVector<uint64_t> Dims;
bool AllIndicesAreConstInt;
Expand Down Expand Up @@ -211,7 +211,7 @@ bool DXILFlattenArraysVisitor::visitAllocaInst(AllocaInst &AI) {

ArrayType *FattenedArrayType = ArrayType::get(BaseType, TotalElements);
AllocaInst *FlatAlloca =
Builder.CreateAlloca(FattenedArrayType, nullptr, AI.getName() + ".flat");
Builder.CreateAlloca(FattenedArrayType, nullptr, AI.getName() + ".1dim");
FlatAlloca->setAlignment(AI.getAlign());
AI.replaceAllUsesWith(FlatAlloca);
AI.eraseFromParent();
Expand All @@ -222,6 +222,10 @@ void DXILFlattenArraysVisitor::recursivelyCollectGEPs(
GetElementPtrInst &CurrGEP, ArrayType *FlattenedArrayType,
Value *PtrOperand, unsigned &GEPChainUseCount, SmallVector<Value *> Indices,
SmallVector<uint64_t> Dims, bool AllIndicesAreConstInt) {
// Check if this GEP is already in the map to avoid circular references
if (GEPChainMap.count(&CurrGEP) > 0)
return;

Value *LastIndex = CurrGEP.getOperand(CurrGEP.getNumOperands() - 1);
AllIndicesAreConstInt &= isa<ConstantInt>(LastIndex);
Indices.push_back(LastIndex);
Expand Down Expand Up @@ -271,10 +275,18 @@ bool DXILFlattenArraysVisitor::visitGetElementPtrInstInGEPChainBase(
genInstructionFlattenIndices(GEPInfo.Indices, GEPInfo.Dims, Builder);

ArrayType *FlattenedArrayType = GEPInfo.ParentArrayType;
Value *FlatGEP =
Builder.CreateGEP(FlattenedArrayType, GEPInfo.ParendOperand,
{Builder.getInt32(0), FlatIndex},
GEP.getName() + ".flat", GEP.getNoWrapFlags());

// Don't append '.flat' to an empty string. If the SSA name isn't available
// it could conflict with the ParentOperand's name.
std::string FlatName = GEP.hasName() ? GEP.getName().str() + ".flat" : "";
Copy link
Member Author

Choose a reason for hiding this comment

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

did .str() in this case because clangd warned that getName() alone could cause a potential use after free.


Value *FlatGEP = Builder.CreateGEP(FlattenedArrayType, GEPInfo.ParentOperand,
{Builder.getInt32(0), FlatIndex}, FlatName,
GEP.getNoWrapFlags());

// Store the new GEP in the map before replacing uses to avoid circular
// references
GEPChainMap.erase(&GEP);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think I understand the comment here with the code correctly.

Is this intended to also insert FlatGEP to GEPChainMap?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. Sorry vestigial comment. Should now say something like erase the old Gep before replacing uses.


GEP.replaceAllUsesWith(FlatGEP);
GEP.eraseFromParent();
Expand Down
6 changes: 4 additions & 2 deletions llvm/test/CodeGen/DirectX/flatten-array.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: [[GEP_PTR:%.*]] = getelementptr inbounds [24 x i32], ptr @a.1dim, i32 0, i32 6
; CHECK-NEXT: load i32, ptr [[GEP_PTR]], 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
Expand Down Expand Up @@ -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: [[GEP_PTR:%.*]] = getelementptr inbounds [24 x i32], ptr @b.1dim, i32 0, i32 13
; CHECK-NEXT: store i32 1, ptr [[GEP_PTR]], align 4
; CHECK-NEXT: ret void
%1 = getelementptr inbounds [2 x [3 x [4 x i32]]], [2 x [3 x [4 x i32]]]* @b, i32 0, i32 1
%2 = getelementptr inbounds [3 x [4 x i32]], [3 x [4 x i32]]* %1, i32 0, i32 0
Expand Down
Loading