Skip to content

Commit 30cd857

Browse files
ManuelJBritoManuelJBrito
committed
Relax conditions safety memory access
1 parent b5657d6 commit 30cd857

File tree

3 files changed

+55
-30
lines changed

3 files changed

+55
-30
lines changed

‎llvm/lib/Transforms/Scalar/NewGVN.cpp

Lines changed: 40 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -2597,34 +2597,61 @@ bool NewGVN::OpIsSafeForPHIOfOps(Value *V, const BasicBlock *PHIBlock,
25972597
Worklist.push_back(V);
25982598
while (!Worklist.empty()) {
25992599
auto *I = Worklist.pop_back_val();
2600-
if (!isa<Instruction>(I))
2600+
if (!(isa<Instruction>(I) || isa<MemoryAccess>(I)))
26012601
continue;
26022602

26032603
auto OISIt = OpSafeForPHIOfOps.find(I);
26042604
if (OISIt != OpSafeForPHIOfOps.end())
26052605
return OISIt->second;
26062606

2607+
// getBlockForValue only works for instructions and memoryPhis.
2608+
auto *IBlock = isa<MemoryUseOrDef>(I) ? cast<MemoryUseOrDef>(I)->getBlock()
2609+
: getBlockForValue(I);
2610+
26072611
// Keep walking until we either dominate the phi block, or hit a phi, or run
26082612
// out of things to check.
2609-
if (DT->properlyDominates(getBlockForValue(I), PHIBlock)) {
2613+
if (DT->properlyDominates(IBlock, PHIBlock)) {
26102614
OpSafeForPHIOfOps.insert({I, true});
26112615
continue;
26122616
}
26132617
// PHI in the same block.
2614-
if (isa<PHINode>(I) && getBlockForValue(I) == PHIBlock) {
2618+
if ((isa<PHINode>(I) || isa<MemoryPhi>(I)) && IBlock == PHIBlock) {
26152619
OpSafeForPHIOfOps.insert({I, false});
26162620
return false;
26172621
}
26182622

2619-
auto *OrigI = cast<Instruction>(I);
2620-
// When we hit an instruction that reads memory (load, call, etc), we must
2621-
// consider any store that may happen in the loop. For now, we assume the
2622-
// worst: there is a store in the loop that alias with this read.
2623-
// The case where the load is outside the loop is already covered by the
2624-
// dominator check above.
2625-
// TODO: relax this condition
2626-
if (OrigI->mayReadFromMemory())
2627-
return false;
2623+
auto *OrigI = dyn_cast<Instruction>(I);
2624+
// When we encounter an instruction that reads memory (load, call, etc.) or
2625+
// a MemoryAccess, we must ensure that it does not depend on a memory Phi in
2626+
// PHIBlock. The base cases are already checked above; now we must check its
2627+
// operands.
2628+
MemoryAccess *MA = nullptr;
2629+
if (OrigI && OrigI->mayReadFromMemory())
2630+
MA = getMemoryAccess(OrigI);
2631+
else if (isa<MemoryAccess>(I))
2632+
MA = cast<MemoryAccess>(I);
2633+
if (MA) {
2634+
for (auto *Op : MA->operand_values()) {
2635+
// Null => LiveOnEntryDef
2636+
if (!Op)
2637+
continue;
2638+
// Stop now if we find an unsafe operand.
2639+
auto OISIt = OpSafeForPHIOfOps.find(OrigI);
2640+
if (OISIt != OpSafeForPHIOfOps.end()) {
2641+
if (!OISIt->second) {
2642+
OpSafeForPHIOfOps.insert({I, false});
2643+
return false;
2644+
}
2645+
continue;
2646+
}
2647+
if (!Visited.insert(Op).second)
2648+
continue;
2649+
Worklist.push_back(Op);
2650+
}
2651+
}
2652+
// If its a MemoryAccess there is nothing more to do.
2653+
if (isa<MemoryAccess>(I))
2654+
continue;
26282655

26292656
// Check the operands of the current instruction.
26302657
for (auto *Op : OrigI->operand_values()) {
@@ -2641,7 +2668,7 @@ bool NewGVN::OpIsSafeForPHIOfOps(Value *V, const BasicBlock *PHIBlock,
26412668
}
26422669
if (!Visited.insert(Op).second)
26432670
continue;
2644-
Worklist.push_back(cast<Instruction>(Op));
2671+
Worklist.push_back(Op);
26452672
}
26462673
}
26472674
OpSafeForPHIOfOps.insert({V, true});

‎llvm/test/Transforms/NewGVN/phi-of-ops-loads.ll

Lines changed: 9 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -83,12 +83,12 @@ define void @no-alias-store-in-loop(ptr noalias %p, ptr noalias %q) {
8383
; CHECK-NEXT: bb56:
8484
; CHECK-NEXT: br label [[BB57:%.*]]
8585
; CHECK: bb57:
86-
; CHECK-NEXT: [[N59:%.*]] = phi i1 [ false, [[BB229:%.*]] ], [ true, [[BB56:%.*]] ]
86+
; CHECK-NEXT: [[PHIOFOPS:%.*]] = phi i1 [ true, [[BB56:%.*]] ], [ [[N62:%.*]], [[BB229:%.*]] ]
87+
; CHECK-NEXT: [[N59:%.*]] = phi i1 [ false, [[BB229]] ], [ true, [[BB56]] ]
8788
; CHECK-NEXT: [[IDX:%.*]] = phi i8 [ 0, [[BB56]] ], [ [[INC:%.*]], [[BB229]] ]
8889
; CHECK-NEXT: [[N60:%.*]] = load i8, ptr [[P:%.*]], align 1
89-
; CHECK-NEXT: [[N62:%.*]] = icmp ne i8 [[N60]], 2
90-
; CHECK-NEXT: [[N63:%.*]] = or i1 [[N59]], [[N62]]
91-
; CHECK-NEXT: br i1 [[N63]], label [[BB229]], label [[BB237:%.*]]
90+
; CHECK-NEXT: [[N62]] = icmp ne i8 [[N60]], 2
91+
; CHECK-NEXT: br i1 [[PHIOFOPS]], label [[BB229]], label [[BB237:%.*]]
9292
; CHECK: bb229:
9393
; CHECK-NEXT: [[INC]] = add i8 [[IDX]], 1
9494
; CHECK-NEXT: store i8 [[INC]], ptr [[Q:%.*]], align 1
@@ -156,11 +156,11 @@ define void @nowrite-function-in-loop(ptr %p) {
156156
; CHECK-NEXT: bb56:
157157
; CHECK-NEXT: br label [[BB57:%.*]]
158158
; CHECK: bb57:
159-
; CHECK-NEXT: [[N59:%.*]] = phi i1 [ false, [[BB229:%.*]] ], [ true, [[BB56:%.*]] ]
159+
; CHECK-NEXT: [[PHIOFOPS:%.*]] = phi i1 [ true, [[BB56:%.*]] ], [ [[N62:%.*]], [[BB229:%.*]] ]
160+
; CHECK-NEXT: [[N59:%.*]] = phi i1 [ false, [[BB229]] ], [ true, [[BB56]] ]
160161
; CHECK-NEXT: [[N60:%.*]] = load i8, ptr [[P:%.*]], align 1
161-
; CHECK-NEXT: [[N62:%.*]] = icmp ne i8 [[N60]], 2
162-
; CHECK-NEXT: [[N63:%.*]] = or i1 [[N59]], [[N62]]
163-
; CHECK-NEXT: br i1 [[N63]], label [[BB229]], label [[BB237:%.*]]
162+
; CHECK-NEXT: [[N62]] = icmp ne i8 [[N60]], 2
163+
; CHECK-NEXT: br i1 [[PHIOFOPS]], label [[BB229]], label [[BB237:%.*]]
164164
; CHECK: bb229:
165165
; CHECK-NEXT: call void @f() #[[ATTR0:[0-9]+]]
166166
; CHECK-NEXT: br label [[BB57]]
@@ -199,9 +199,7 @@ define void @issfeoperand(ptr nocapture readonly %array, i1 %cond1, i1 %cond2, p
199199
; CHECK-NEXT: [[PHI1:%.*]] = phi i8 [ [[LD1]], [[COND_TRUE]] ], [ 0, [[FOR_BODY:%.*]] ]
200200
; CHECK-NEXT: [[ARRAYIDX42:%.*]] = getelementptr inbounds [3 x [2 x [1 x i8]]], ptr [[ARRAY]], i64 109, i64 0, i64 0, i64 undef
201201
; CHECK-NEXT: [[LD2:%.*]] = load i8, ptr [[ARRAYIDX42]], align 1
202-
; CHECK-NEXT: [[CMP1:%.*]] = icmp ult i8 [[LD2]], [[PHI1]]
203-
; CHECK-NEXT: [[ZEXT:%.*]] = zext i1 [[CMP1]] to i32
204-
; CHECK-NEXT: store i32 [[ZEXT]], ptr [[P2:%.*]], align 4
202+
; CHECK-NEXT: store i32 0, ptr [[P2:%.*]], align 4
205203
; CHECK-NEXT: br i1 [[COND2:%.*]], label [[COND_END:%.*]], label [[EXIT:%.*]]
206204
; CHECK: cond.end:
207205
; CHECK-NEXT: [[LD3:%.*]] = load i16, ptr [[P1:%.*]], align 2

‎llvm/test/Transforms/NewGVN/storeoverstore.ll

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -15,13 +15,13 @@ define i32 @foo(ptr, i32) {
1515
; CHECK: 4:
1616
; CHECK-NEXT: br label [[TMP5]]
1717
; CHECK: 5:
18-
; CHECK-NEXT: [[DOT0:%.*]] = phi i32 [ 10, [[TMP4]] ], [ 5, [[TMP2:%.*]] ]
19-
; CHECK-NEXT: br i1 [[TMP3]], label [[TMP6:%.*]], label [[TMP8:%.*]]
18+
; CHECK-NEXT: [[PHIOFOPS:%.*]] = phi i32 [ 10, [[TMP2:%.*]] ], [ 15, [[TMP4]] ]
19+
; CHECK-NEXT: [[DOT0:%.*]] = phi i32 [ 10, [[TMP4]] ], [ 5, [[TMP2]] ]
20+
; CHECK-NEXT: br i1 [[TMP3]], label [[TMP6:%.*]], label [[TMP7:%.*]]
2021
; CHECK: 6:
21-
; CHECK-NEXT: [[TMP7:%.*]] = add nsw i32 [[DOT0]], 5
22-
; CHECK-NEXT: br label [[TMP8]]
23-
; CHECK: 8:
24-
; CHECK-NEXT: [[DOT1:%.*]] = phi i32 [ [[TMP7]], [[TMP6]] ], [ [[DOT0]], [[TMP5]] ]
22+
; CHECK-NEXT: br label [[TMP7]]
23+
; CHECK: 7:
24+
; CHECK-NEXT: [[DOT1:%.*]] = phi i32 [ [[PHIOFOPS]], [[TMP6]] ], [ [[DOT0]], [[TMP5]] ]
2525
; CHECK-NEXT: ret i32 [[DOT1]]
2626
;
2727
store i32 5, ptr %0, align 4

0 commit comments

Comments
 (0)