Skip to content

Commit

Permalink
[MSSA] Don't require clone creation to succeed (llvm#76819)
Browse files Browse the repository at this point in the history
Sometimes, we create a MemoryAccess for an instruction, which is later
simplified (e.g. via devirtualization) such that the new instruction has
no memory effects anymore.

If we later clone the instruction (e.g. during unswitching), then MSSA
will not create a MemoryAccess for the new instruction, triggering an
assert.

Disable the assertion (by passing CreationMustSucceed=false) and adjust
getDefiningAccessForClone() to work correctly in that case.

This PR implements the alternative suggestion by alinas from
llvm#76142.

(cherry picked from commit d02c793)
  • Loading branch information
nikic authored and MingcongBai committed Mar 26, 2024
1 parent f03a987 commit 271282c
Show file tree
Hide file tree
Showing 2 changed files with 121 additions and 13 deletions.
17 changes: 4 additions & 13 deletions llvm/lib/Analysis/MemorySSAUpdater.cpp
Expand Up @@ -568,7 +568,6 @@ static MemoryAccess *onlySingleValue(MemoryPhi *MP) {
static MemoryAccess *getNewDefiningAccessForClone(MemoryAccess *MA,
const ValueToValueMapTy &VMap,
PhiToDefMap &MPhiMap,
bool CloneWasSimplified,
MemorySSA *MSSA) {
MemoryAccess *InsnDefining = MA;
if (MemoryDef *DefMUD = dyn_cast<MemoryDef>(InsnDefining)) {
Expand All @@ -578,18 +577,10 @@ static MemoryAccess *getNewDefiningAccessForClone(MemoryAccess *MA,
if (Instruction *NewDefMUDI =
cast_or_null<Instruction>(VMap.lookup(DefMUDI))) {
InsnDefining = MSSA->getMemoryAccess(NewDefMUDI);
if (!CloneWasSimplified)
assert(InsnDefining && "Defining instruction cannot be nullptr.");
else if (!InsnDefining || isa<MemoryUse>(InsnDefining)) {
if (!InsnDefining || isa<MemoryUse>(InsnDefining)) {
// The clone was simplified, it's no longer a MemoryDef, look up.
auto DefIt = DefMUD->getDefsIterator();
// Since simplified clones only occur in single block cloning, a
// previous definition must exist, otherwise NewDefMUDI would not
// have been found in VMap.
assert(DefIt != MSSA->getBlockDefs(DefMUD->getBlock())->begin() &&
"Previous def must exist");
InsnDefining = getNewDefiningAccessForClone(
&*(--DefIt), VMap, MPhiMap, CloneWasSimplified, MSSA);
DefMUD->getDefiningAccess(), VMap, MPhiMap, MSSA);
}
}
}
Expand Down Expand Up @@ -624,9 +615,9 @@ void MemorySSAUpdater::cloneUsesAndDefs(BasicBlock *BB, BasicBlock *NewBB,
MemoryAccess *NewUseOrDef = MSSA->createDefinedAccess(
NewInsn,
getNewDefiningAccessForClone(MUD->getDefiningAccess(), VMap,
MPhiMap, CloneWasSimplified, MSSA),
MPhiMap, MSSA),
/*Template=*/CloneWasSimplified ? nullptr : MUD,
/*CreationMustSucceed=*/CloneWasSimplified ? false : true);
/*CreationMustSucceed=*/false);
if (NewUseOrDef)
MSSA->insertIntoListsForBlock(NewUseOrDef, NewBB, MemorySSA::End);
}
Expand Down
117 changes: 117 additions & 0 deletions llvm/test/Transforms/SimpleLoopUnswitch/memssa-readnone-access.ll
@@ -0,0 +1,117 @@
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py UTC_ARGS: --version 4
; RUN: opt -S -passes="loop-mssa(loop-instsimplify,simple-loop-unswitch<nontrivial>)" < %s | FileCheck %s

@vtable = constant ptr @foo

declare void @foo() memory(none)
declare void @bar()

; The call becomes known readnone after simplification, but still have a
; MemoryAccess. Make sure this does not lead to an assertion failure.
define void @test(i1 %c) {
; CHECK-LABEL: define void @test(
; CHECK-SAME: i1 [[C:%.*]]) {
; 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: br label [[EXIT_SPLIT_US:%.*]]
; CHECK: exit.split.us:
; CHECK-NEXT: br label [[EXIT:%.*]]
; CHECK: .split:
; CHECK-NEXT: br label [[LOOP:%.*]]
; CHECK: loop:
; CHECK-NEXT: call void @foo()
; CHECK-NEXT: br label [[LOOP]]
; CHECK: exit:
; CHECK-NEXT: ret void
;
br label %loop

loop:
%fn = load ptr, ptr @vtable, align 8
call void %fn()
br i1 %c, label %exit, label %loop

exit:
ret void
}

; Variant with another access after the call.
define void @test2(i1 %c, ptr %p) {
; CHECK-LABEL: define void @test2(
; 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 [[EXIT_SPLIT_US:%.*]]
; CHECK: exit.split.us:
; CHECK-NEXT: br label [[EXIT:%.*]]
; CHECK: .split:
; CHECK-NEXT: br label [[LOOP:%.*]]
; CHECK: loop:
; CHECK-NEXT: call void @foo()
; CHECK-NEXT: call void @bar()
; CHECK-NEXT: br label [[LOOP]]
; 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 %exit, label %loop

exit:
ret void
}

; Variant with another access after the call and no access before the call.
define void @test3(i1 %c, ptr %p) {
; CHECK-LABEL: define void @test3(
; 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 [[EXIT_SPLIT_US:%.*]]
; CHECK: exit.split.us:
; CHECK-NEXT: br label [[EXIT:%.*]]
; 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 [[LOOP]]
; 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 %exit, label %loop

exit:
ret void
}

0 comments on commit 271282c

Please sign in to comment.