From 373adeb03cfcec19251fe2e3d6e617ecf6c4ddeb Mon Sep 17 00:00:00 2001 From: Nikita Popov Date: Wed, 27 Nov 2024 12:56:24 +0100 Subject: [PATCH 1/2] [MemorySSA] Handle MemoryDef optimized away during cloning When determining the replacement access during cloning, we currently leave accesses for instructions that are not in the VMap alone. This is correct if the instruction is not in VMap because it hasn't been cloned, but not if it has been cloned and then removed. In that case, we should walk up to the defining access, like in other simplification cases. To distinguish the two cases, pass in a callback that queries where the instruction is part of the cloned region. An alternative to this would be to delay removal of dead instructions in SimpleLoopUnswitch until after MSSA cloning. --- llvm/include/llvm/Analysis/MemorySSAUpdater.h | 2 + llvm/lib/Analysis/MemorySSAUpdater.cpp | 68 +++++++++++-------- .../Analysis/MemorySSA/loop-rotate-update.ll | 37 ++++++++++ llvm/test/Analysis/MemorySSA/pr116227.ll | 61 +++++++++++++++++ 4 files changed, 139 insertions(+), 29 deletions(-) create mode 100644 llvm/test/Analysis/MemorySSA/loop-rotate-update.ll create mode 100644 llvm/test/Analysis/MemorySSA/pr116227.ll diff --git a/llvm/include/llvm/Analysis/MemorySSAUpdater.h b/llvm/include/llvm/Analysis/MemorySSAUpdater.h index 055feceefb054..b925ddd5de12a 100644 --- a/llvm/include/llvm/Analysis/MemorySSAUpdater.h +++ b/llvm/include/llvm/Analysis/MemorySSAUpdater.h @@ -276,7 +276,9 @@ class MemorySSAUpdater { // representation for. No other cases are supported. void cloneUsesAndDefs(BasicBlock *BB, BasicBlock *NewBB, const ValueToValueMapTy &VMap, PhiToDefMap &MPhiMap, + function_ref IsInClonedRegion, bool CloneWasSimplified = false); + template void privateUpdateExitBlocksForClonedLoop(ArrayRef ExitBlocks, Iter ValuesBegin, Iter ValuesEnd, diff --git a/llvm/lib/Analysis/MemorySSAUpdater.cpp b/llvm/lib/Analysis/MemorySSAUpdater.cpp index f672bd0e1e133..050b827388d3c 100644 --- a/llvm/lib/Analysis/MemorySSAUpdater.cpp +++ b/llvm/lib/Analysis/MemorySSAUpdater.cpp @@ -565,24 +565,26 @@ static MemoryAccess *onlySingleValue(MemoryPhi *MP) { return MA; } -static MemoryAccess *getNewDefiningAccessForClone(MemoryAccess *MA, - const ValueToValueMapTy &VMap, - PhiToDefMap &MPhiMap, - MemorySSA *MSSA) { +static MemoryAccess *getNewDefiningAccessForClone( + MemoryAccess *MA, const ValueToValueMapTy &VMap, PhiToDefMap &MPhiMap, + MemorySSA *MSSA, function_ref IsInClonedRegion) { MemoryAccess *InsnDefining = MA; if (MemoryDef *DefMUD = dyn_cast(InsnDefining)) { - if (!MSSA->isLiveOnEntryDef(DefMUD)) { - Instruction *DefMUDI = DefMUD->getMemoryInst(); - assert(DefMUDI && "Found MemoryUseOrDef with no Instruction."); - if (Instruction *NewDefMUDI = - cast_or_null(VMap.lookup(DefMUDI))) { - InsnDefining = MSSA->getMemoryAccess(NewDefMUDI); - if (!InsnDefining || isa(InsnDefining)) { - // The clone was simplified, it's no longer a MemoryDef, look up. - InsnDefining = getNewDefiningAccessForClone( - DefMUD->getDefiningAccess(), VMap, MPhiMap, MSSA); - } - } + if (MSSA->isLiveOnEntryDef(DefMUD)) + return DefMUD; + + // If the MemoryDef is not part of the cloned region, leave it alone. + Instruction *DefMUDI = DefMUD->getMemoryInst(); + assert(DefMUDI && "Found MemoryUseOrDef with no Instruction."); + if (!IsInClonedRegion(DefMUDI->getParent())) + return DefMUD; + + auto *NewDefMUDI = cast_or_null(VMap.lookup(DefMUDI)); + InsnDefining = NewDefMUDI ? MSSA->getMemoryAccess(NewDefMUDI) : nullptr; + if (!InsnDefining || isa(InsnDefining)) { + // The clone was simplified, it's no longer a MemoryDef, look up. + InsnDefining = getNewDefiningAccessForClone( + DefMUD->getDefiningAccess(), VMap, MPhiMap, MSSA, IsInClonedRegion); } } else { MemoryPhi *DefPhi = cast(InsnDefining); @@ -593,10 +595,10 @@ static MemoryAccess *getNewDefiningAccessForClone(MemoryAccess *MA, return InsnDefining; } -void MemorySSAUpdater::cloneUsesAndDefs(BasicBlock *BB, BasicBlock *NewBB, - const ValueToValueMapTy &VMap, - PhiToDefMap &MPhiMap, - bool CloneWasSimplified) { +void MemorySSAUpdater::cloneUsesAndDefs( + BasicBlock *BB, BasicBlock *NewBB, const ValueToValueMapTy &VMap, + PhiToDefMap &MPhiMap, function_ref IsInClonedRegion, + bool CloneWasSimplified) { const MemorySSA::AccessList *Acc = MSSA->getBlockAccesses(BB); if (!Acc) return; @@ -615,7 +617,7 @@ void MemorySSAUpdater::cloneUsesAndDefs(BasicBlock *BB, BasicBlock *NewBB, MemoryAccess *NewUseOrDef = MSSA->createDefinedAccess( NewInsn, getNewDefiningAccessForClone(MUD->getDefiningAccess(), VMap, - MPhiMap, MSSA), + MPhiMap, MSSA, IsInClonedRegion), /*Template=*/CloneWasSimplified ? nullptr : MUD, /*CreationMustSucceed=*/false); if (NewUseOrDef) @@ -668,8 +670,13 @@ void MemorySSAUpdater::updateForClonedLoop(const LoopBlocksRPO &LoopBlocks, ArrayRef ExitBlocks, const ValueToValueMapTy &VMap, bool IgnoreIncomingWithNoClones) { - PhiToDefMap MPhiMap; + SmallSetVector Blocks; + for (BasicBlock *BB : concat(LoopBlocks, ExitBlocks)) + Blocks.insert(BB); + auto IsInClonedRegion = [&](BasicBlock *BB) { return Blocks.contains(BB); }; + + PhiToDefMap MPhiMap; auto FixPhiIncomingValues = [&](MemoryPhi *Phi, MemoryPhi *NewPhi) { assert(Phi && NewPhi && "Invalid Phi nodes."); BasicBlock *NewPhiBB = NewPhi->getBlock(); @@ -692,9 +699,10 @@ void MemorySSAUpdater::updateForClonedLoop(const LoopBlocksRPO &LoopBlocks, continue; // Determine incoming value and add it as incoming from IncBB. - NewPhi->addIncoming( - getNewDefiningAccessForClone(IncomingAccess, VMap, MPhiMap, MSSA), - IncBB); + NewPhi->addIncoming(getNewDefiningAccessForClone(IncomingAccess, VMap, + MPhiMap, MSSA, + IsInClonedRegion), + IncBB); } if (auto *SingleAccess = onlySingleValue(NewPhi)) { MPhiMap[Phi] = SingleAccess; @@ -716,13 +724,13 @@ void MemorySSAUpdater::updateForClonedLoop(const LoopBlocksRPO &LoopBlocks, MPhiMap[MPhi] = NewPhi; } // Update Uses and Defs. - cloneUsesAndDefs(BB, NewBlock, VMap, MPhiMap); + cloneUsesAndDefs(BB, NewBlock, VMap, MPhiMap, IsInClonedRegion); }; - for (auto *BB : llvm::concat(LoopBlocks, ExitBlocks)) + for (auto *BB : Blocks) ProcessBlock(BB); - for (auto *BB : llvm::concat(LoopBlocks, ExitBlocks)) + for (auto *BB : Blocks) if (MemoryPhi *MPhi = MSSA->getMemoryAccess(BB)) if (MemoryAccess *NewPhi = MPhiMap.lookup(MPhi)) FixPhiIncomingValues(MPhi, cast(NewPhi)); @@ -741,7 +749,9 @@ void MemorySSAUpdater::updateForClonedBlockIntoPred( PhiToDefMap MPhiMap; if (MemoryPhi *MPhi = MSSA->getMemoryAccess(BB)) MPhiMap[MPhi] = MPhi->getIncomingValueForBlock(P1); - cloneUsesAndDefs(BB, P1, VM, MPhiMap, /*CloneWasSimplified=*/true); + cloneUsesAndDefs( + BB, P1, VM, MPhiMap, [&](BasicBlock *CheckBB) { return BB == CheckBB; }, + /*CloneWasSimplified=*/true); } template diff --git a/llvm/test/Analysis/MemorySSA/loop-rotate-update.ll b/llvm/test/Analysis/MemorySSA/loop-rotate-update.ll new file mode 100644 index 0000000000000..4ed4e825af569 --- /dev/null +++ b/llvm/test/Analysis/MemorySSA/loop-rotate-update.ll @@ -0,0 +1,37 @@ +; RUN: opt -disable-output -passes="loop-mssa(loop-rotate),print" -verify-memoryssa < %s 2>&1 | FileCheck %s + +; CHECK: entry: +; CHECK-NEXT: 3 = MemoryDef(liveOnEntry) +; CHECK-NEXT: store ptr null, ptr %p, align 8 +; CHECK-NEXT: MemoryUse(3) +; CHECK-NEXT: %val11 = load ptr, ptr %p, align 8 + +; CHECK: loop.latch: +; CHECK-NEXT: 5 = MemoryPhi({loop.latch,1},{loop.latch.lr.ph,3}) +; CHECK-NEXT: MemoryUse(5) +; CHECK-NEXT: %val2 = load ptr, ptr %p, align 8 +; CHECK-NEXT: 1 = MemoryDef(5) +; CHECK-NEXT: store ptr null, ptr %p, align 8 +; CHECK-NEXT: MemoryUse(1) +; CHECK-NEXT: %val1 = load ptr, ptr %p, align 8 + +; CHECK: exit: +; CHECK-NEXT: 4 = MemoryPhi({entry,3},{loop.exit_crit_edge,1}) + +define void @test(ptr %p) { +entry: + br label %loop + +loop: + store ptr null, ptr %p + %val1 = load ptr, ptr %p + %cmp = icmp eq ptr %val1, null + br i1 %cmp, label %exit, label %loop.latch + +loop.latch: + %val2 = load ptr, ptr %p + br label %loop + +exit: + ret void +} diff --git a/llvm/test/Analysis/MemorySSA/pr116227.ll b/llvm/test/Analysis/MemorySSA/pr116227.ll new file mode 100644 index 0000000000000..cd9686b018aa6 --- /dev/null +++ b/llvm/test/Analysis/MemorySSA/pr116227.ll @@ -0,0 +1,61 @@ +; RUN: opt -disable-output -passes="loop-mssa(simple-loop-unswitch),print" -verify-memoryssa < %s 2>&1 | FileCheck %s + +declare ptr @malloc() allockind("alloc,uninitialized") + +; CHECK-LABEL: MemorySSA for function: test + +; CHECK: for.body.us: +; CHECK-NEXT: 3 = MemoryPhi({entry.split.us,liveOnEntry},{for.body.us,3}) + +; CHECK: for.body: +; CHECK-NEXT: 2 = MemoryPhi({entry.split,liveOnEntry},{for.body,1}) +; CHECK-NEXT: 1 = MemoryDef(2) +; CHECK-NEXT: %call.i = call ptr @malloc() + +define void @test(i1 %arg) { +entry: + br label %for.body + +for.body: + %call.i = call ptr @malloc() + %cmp.i = icmp ne ptr %call.i, null + %or.cond.i = select i1 %cmp.i, i1 %arg, i1 false + br i1 %or.cond.i, label %exit, label %for.body + +exit: + ret void +} + +; CHECK-LABEL: MemorySSA for function: test_extra_defs + +; CHECK: entry: +; CHECK-NEXT: 1 = MemoryDef(liveOnEntry) +; CHECK-NEXT: store i8 1, ptr %p, align 1 + +; CHECK: for.body.us: +; CHECK-NEXT: 5 = MemoryPhi({entry.split.us,1},{for.body.us,6}) +; CHECK-NEXT: 6 = MemoryDef(5) +; CHECK-NEXT: store i8 2, ptr %p, align 1 + +; CHECK: for.body: +; CHECK-NEXT: 4 = MemoryPhi({entry.split,1},{for.body,3}) +; CHECK-NEXT: 2 = MemoryDef(4) +; CHECK-NEXT: store i8 2, ptr %p +; CHECK-NEXT: 3 = MemoryDef(2) +; CHECK-NEXT: %call.i = call ptr @malloc() + +define void @test_extra_defs(ptr %p, i1 %arg) { +entry: + store i8 1, ptr %p + br label %for.body + +for.body: + store i8 2, ptr %p + %call.i = call ptr @malloc() + %cmp.i = icmp ne ptr %call.i, null + %or.cond.i = select i1 %cmp.i, i1 %arg, i1 false + br i1 %or.cond.i, label %exit, label %for.body + +exit: + ret void +} From e77e72ccd6e58344c5ff66d447e62d75a94eb916 Mon Sep 17 00:00:00 2001 From: Nikita Popov Date: Wed, 4 Dec 2024 16:02:24 +0100 Subject: [PATCH 2/2] Add docs for IsInClonedRegion param --- llvm/include/llvm/Analysis/MemorySSAUpdater.h | 35 +++++++++++-------- 1 file changed, 21 insertions(+), 14 deletions(-) diff --git a/llvm/include/llvm/Analysis/MemorySSAUpdater.h b/llvm/include/llvm/Analysis/MemorySSAUpdater.h index b925ddd5de12a..b8e08f4b7842f 100644 --- a/llvm/include/llvm/Analysis/MemorySSAUpdater.h +++ b/llvm/include/llvm/Analysis/MemorySSAUpdater.h @@ -260,20 +260,27 @@ class MemorySSAUpdater { MemoryAccess *tryRemoveTrivialPhi(MemoryPhi *Phi, RangeType &Operands); void tryRemoveTrivialPhis(ArrayRef UpdatedPHIs); void fixupDefs(const SmallVectorImpl &); - // Clone all uses and defs from BB to NewBB given a 1:1 map of all - // instructions and blocks cloned, and a map of MemoryPhi : Definition - // (MemoryAccess Phi or Def). VMap maps old instructions to cloned - // instructions and old blocks to cloned blocks. MPhiMap, is created in the - // caller of this private method, and maps existing MemoryPhis to new - // definitions that new MemoryAccesses must point to. These definitions may - // not necessarily be MemoryPhis themselves, they may be MemoryDefs. As such, - // the map is between MemoryPhis and MemoryAccesses, where the MemoryAccesses - // may be MemoryPhis or MemoryDefs and not MemoryUses. - // If CloneWasSimplified = true, the clone was exact. Otherwise, assume that - // the clone involved simplifications that may have: (1) turned a MemoryUse - // into an instruction that MemorySSA has no representation for, or (2) turned - // a MemoryDef into a MemoryUse or an instruction that MemorySSA has no - // representation for. No other cases are supported. + /// Clone all uses and defs from BB to NewBB given a 1:1 map of all + /// instructions and blocks cloned, and a map of MemoryPhi : Definition + /// (MemoryAccess Phi or Def). + /// + /// \param VMap Maps old instructions to cloned instructions and old blocks + /// to cloned blocks + /// \param MPhiMap, is created in the caller of this private method, and maps + /// existing MemoryPhis to new definitions that new MemoryAccesses + /// must point to. These definitions may not necessarily be MemoryPhis + /// themselves, they may be MemoryDefs. As such, the map is between + /// MemoryPhis and MemoryAccesses, where the MemoryAccesses may be + /// MemoryPhis or MemoryDefs and not MemoryUses. + /// \param IsInClonedRegion Determines whether a basic block was cloned. + /// References to accesses outside the cloned region will not be + /// remapped. + /// \param CloneWasSimplified If false, the clone was exact. Otherwise, + /// assume that the clone involved simplifications that may have: + /// (1) turned a MemoryUse into an instruction that MemorySSA has no + /// representation for, or (2) turned a MemoryDef into a MemoryUse or + /// an instruction that MemorySSA has no representation for. No other + /// cases are supported. void cloneUsesAndDefs(BasicBlock *BB, BasicBlock *NewBB, const ValueToValueMapTy &VMap, PhiToDefMap &MPhiMap, function_ref IsInClonedRegion,