-
Notifications
You must be signed in to change notification settings - Fork 11.8k
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
PR for llvm/llvm-project#79571 #79572
Conversation
@alinas What do you think about merging this PR to the release branch? |
@llvm/pr-subscribers-llvm-transforms @llvm/pr-subscribers-llvm-analysis Author: None (github-actions[bot]) Changesresolves llvm/llvm-project#79571 Full diff: https://github.com/llvm/llvm-project/pull/79572.diff 2 Files Affected:
diff --git a/llvm/lib/Analysis/MemorySSAUpdater.cpp b/llvm/lib/Analysis/MemorySSAUpdater.cpp
index e87ae7d71fffe20..aa550f0b6a7bfd6 100644
--- a/llvm/lib/Analysis/MemorySSAUpdater.cpp
+++ b/llvm/lib/Analysis/MemorySSAUpdater.cpp
@@ -692,25 +692,9 @@ void MemorySSAUpdater::updateForClonedLoop(const LoopBlocksRPO &LoopBlocks,
continue;
// Determine incoming value and add it as incoming from IncBB.
- if (MemoryUseOrDef *IncMUD = dyn_cast<MemoryUseOrDef>(IncomingAccess)) {
- if (!MSSA->isLiveOnEntryDef(IncMUD)) {
- Instruction *IncI = IncMUD->getMemoryInst();
- assert(IncI && "Found MemoryUseOrDef with no Instruction.");
- if (Instruction *NewIncI =
- cast_or_null<Instruction>(VMap.lookup(IncI))) {
- IncMUD = MSSA->getMemoryAccess(NewIncI);
- assert(IncMUD &&
- "MemoryUseOrDef cannot be null, all preds processed.");
- }
- }
- NewPhi->addIncoming(IncMUD, IncBB);
- } else {
- MemoryPhi *IncPhi = cast<MemoryPhi>(IncomingAccess);
- if (MemoryAccess *NewDefPhi = MPhiMap.lookup(IncPhi))
- NewPhi->addIncoming(NewDefPhi, IncBB);
- else
- NewPhi->addIncoming(IncPhi, IncBB);
- }
+ NewPhi->addIncoming(
+ getNewDefiningAccessForClone(IncomingAccess, VMap, MPhiMap, MSSA),
+ IncBB);
}
if (auto *SingleAccess = onlySingleValue(NewPhi)) {
MPhiMap[Phi] = SingleAccess;
diff --git a/llvm/test/Transforms/SimpleLoopUnswitch/memssa-readnone-access.ll b/llvm/test/Transforms/SimpleLoopUnswitch/memssa-readnone-access.ll
index 2aaf777683e116f..c6e6608d4be383a 100644
--- a/llvm/test/Transforms/SimpleLoopUnswitch/memssa-readnone-access.ll
+++ b/llvm/test/Transforms/SimpleLoopUnswitch/memssa-readnone-access.ll
@@ -115,3 +115,107 @@ split:
exit:
ret void
}
+
+; Variants of the above test with swapped branch destinations.
+
+define void @test1_swapped(i1 %c) {
+; CHECK-LABEL: define void @test1_swapped(
+; CHECK-SAME: i1 [[C:%.*]]) {
+; CHECK-NEXT: start:
+; CHECK-NEXT: [[C_FR:%.*]] = freeze i1 [[C]]
+; CHECK-NEXT: br i1 [[C_FR]], label [[START_SPLIT_US:%.*]], label [[START_SPLIT:%.*]]
+; CHECK: start.split.us:
+; CHECK-NEXT: br label [[LOOP_US:%.*]]
+; CHECK: loop.us:
+; CHECK-NEXT: call void @foo()
+; CHECK-NEXT: br label [[LOOP_US]]
+; CHECK: start.split:
+; CHECK-NEXT: br label [[LOOP:%.*]]
+; CHECK: loop:
+; CHECK-NEXT: call void @foo()
+; CHECK-NEXT: br label [[EXIT:%.*]]
+; CHECK: exit:
+; CHECK-NEXT: ret void
+;
+start:
+ br label %loop
+
+loop:
+ %fn = load ptr, ptr @vtable, align 8
+ call void %fn()
+ br i1 %c, label %loop, label %exit
+
+exit:
+ ret void
+}
+
+define void @test2_swapped(i1 %c, ptr %p) {
+; CHECK-LABEL: define void @test2_swapped(
+; CHECK-SAME: i1 [[C:%.*]], ptr [[P:%.*]]) {
+; CHECK-NEXT: [[C_FR:%.*]] = freeze i1 [[C]]
+; CHECK-NEXT: br i1 [[C_FR]], label [[DOTSPLIT_US:%.*]], label [[DOTSPLIT:%.*]]
+; CHECK: .split.us:
+; CHECK-NEXT: br label [[LOOP_US:%.*]]
+; CHECK: loop.us:
+; CHECK-NEXT: call void @foo()
+; CHECK-NEXT: call void @bar()
+; CHECK-NEXT: br label [[LOOP_US]]
+; CHECK: .split:
+; CHECK-NEXT: br label [[LOOP:%.*]]
+; CHECK: loop:
+; CHECK-NEXT: call void @foo()
+; CHECK-NEXT: call void @bar()
+; CHECK-NEXT: br label [[EXIT:%.*]]
+; CHECK: exit:
+; CHECK-NEXT: ret void
+;
+ br label %loop
+
+loop:
+ %fn = load ptr, ptr @vtable, align 8
+ call void %fn()
+ call void @bar()
+ br i1 %c, label %loop, label %exit
+
+exit:
+ ret void
+}
+
+define void @test3_swapped(i1 %c, ptr %p) {
+; CHECK-LABEL: define void @test3_swapped(
+; CHECK-SAME: i1 [[C:%.*]], ptr [[P:%.*]]) {
+; CHECK-NEXT: [[C_FR:%.*]] = freeze i1 [[C]]
+; CHECK-NEXT: br i1 [[C_FR]], label [[DOTSPLIT_US:%.*]], label [[DOTSPLIT:%.*]]
+; CHECK: .split.us:
+; CHECK-NEXT: br label [[LOOP_US:%.*]]
+; CHECK: loop.us:
+; CHECK-NEXT: br label [[SPLIT_US:%.*]]
+; CHECK: split.us:
+; CHECK-NEXT: call void @foo()
+; CHECK-NEXT: call void @bar()
+; CHECK-NEXT: br label [[LOOP_US]]
+; CHECK: .split:
+; CHECK-NEXT: br label [[LOOP:%.*]]
+; CHECK: loop:
+; CHECK-NEXT: br label [[SPLIT:%.*]]
+; CHECK: split:
+; CHECK-NEXT: call void @foo()
+; CHECK-NEXT: call void @bar()
+; CHECK-NEXT: br label [[EXIT:%.*]]
+; CHECK: exit:
+; CHECK-NEXT: ret void
+;
+ br label %loop
+
+loop:
+ %fn = load ptr, ptr @vtable, align 8
+ br label %split
+
+split:
+ call void %fn()
+ call void @bar()
+ br i1 %c, label %loop, label %exit
+
+exit:
+ ret void
+}
|
@alinas What do you think about merging this PR to the release branch? |
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.
There are some pre-merge failures to review, but including this in the release makes sense.
This is a followup to llvm#76819. After those changes, we can still run into an assertion failure for a slight variation of the test case: When fixing up MemoryPhis, we map the incoming access to the access of the cloned instruction -- which may now no longer exist. Fix this by reusing the getNewDefiningAccessForClone() helper, which will look upwards for a new defining access in that case. (cherry picked from commit a7a1b8b)
resolves #79571