From cb03470aefb91dfd0d01f2651d604e9afaef5cb1 Mon Sep 17 00:00:00 2001 From: Nikita Popov Date: Tue, 27 Dec 2022 12:17:58 +0100 Subject: [PATCH] Reapply [MergeLoadStoreMotion] Don't require GEP for sinking Reapply with a fix for a failing debuginfo assignment tracking test. ----- Allow sinking stores where both operands are the same, don't require them to have an identical GEP in each block. This came up when migrating tests to opaque pointers, where zero-index GEPs are omitted. --- .../Scalar/MergedLoadStoreMotion.cpp | 44 ++++++++++--------- .../MergedLoadStoreMotion/st_sink_no_gep.ll | 8 ++-- .../MergedLoadStoreMotion/st_sink_split_bb.ll | 4 +- 3 files changed, 30 insertions(+), 26 deletions(-) diff --git a/llvm/lib/Transforms/Scalar/MergedLoadStoreMotion.cpp b/llvm/lib/Transforms/Scalar/MergedLoadStoreMotion.cpp index 253a850d9479..62e75d98448c 100644 --- a/llvm/lib/Transforms/Scalar/MergedLoadStoreMotion.cpp +++ b/llvm/lib/Transforms/Scalar/MergedLoadStoreMotion.cpp @@ -220,27 +220,29 @@ PHINode *MergedLoadStoreMotion::getPHIOperand(BasicBlock *BB, StoreInst *S0, } /// -/// Check if 2 stores can be sunk together with corresponding GEPs +/// Check if 2 stores can be sunk, optionally together with corresponding GEPs. /// bool MergedLoadStoreMotion::canSinkStoresAndGEPs(StoreInst *S0, StoreInst *S1) const { - auto *A0 = dyn_cast(S0->getPointerOperand()); - auto *A1 = dyn_cast(S1->getPointerOperand()); - return A0 && A1 && A0->isIdenticalTo(A1) && A0->hasOneUse() && - (A0->getParent() == S0->getParent()) && A1->hasOneUse() && - (A1->getParent() == S1->getParent()) && isa(A0); + if (S0->getPointerOperand() == S1->getPointerOperand()) + return true; + auto *GEP0 = dyn_cast(S0->getPointerOperand()); + auto *GEP1 = dyn_cast(S1->getPointerOperand()); + return GEP0 && GEP1 && GEP0->isIdenticalTo(GEP1) && GEP0->hasOneUse() && + (GEP0->getParent() == S0->getParent()) && GEP1->hasOneUse() && + (GEP1->getParent() == S1->getParent()); } /// /// Merge two stores to same address and sink into \p BB /// -/// Also sinks GEP instruction computing the store address +/// Optionally also sinks GEP instruction computing the store address /// void MergedLoadStoreMotion::sinkStoresAndGEPs(BasicBlock *BB, StoreInst *S0, StoreInst *S1) { + Value *Ptr0 = S0->getPointerOperand(); + Value *Ptr1 = S1->getPointerOperand(); // Only one definition? - auto *A0 = dyn_cast(S0->getPointerOperand()); - auto *A1 = dyn_cast(S1->getPointerOperand()); LLVM_DEBUG(dbgs() << "Sink Instruction into BB \n"; BB->dump(); dbgs() << "Instruction Left\n"; S0->dump(); dbgs() << "\n"; dbgs() << "Instruction Right\n"; S1->dump(); dbgs() << "\n"); @@ -254,23 +256,25 @@ void MergedLoadStoreMotion::sinkStoresAndGEPs(BasicBlock *BB, StoreInst *S0, // Create the new store to be inserted at the join point. StoreInst *SNew = cast(S0->clone()); - Instruction *ANew = A0->clone(); SNew->insertBefore(&*InsertPt); - ANew->insertBefore(SNew); - ANew->applyMergedLocation(A0->getDebugLoc(), A1->getDebugLoc()); - - assert(S0->getParent() == A0->getParent()); - assert(S1->getParent() == A1->getParent()); - // New PHI operand? Use it. if (PHINode *NewPN = getPHIOperand(BB, S0, S1)) SNew->setOperand(0, NewPN); S0->eraseFromParent(); S1->eraseFromParent(); - A0->replaceAllUsesWith(ANew); - A0->eraseFromParent(); - A1->replaceAllUsesWith(ANew); - A1->eraseFromParent(); + + if (Ptr0 != Ptr1) { + auto *GEP0 = cast(Ptr0); + auto *GEP1 = cast(Ptr1); + Instruction *GEPNew = GEP0->clone(); + GEPNew->insertBefore(SNew); + GEPNew->applyMergedLocation(GEP0->getDebugLoc(), GEP1->getDebugLoc()); + SNew->setOperand(1, GEPNew); + GEP0->replaceAllUsesWith(GEPNew); + GEP0->eraseFromParent(); + GEP1->replaceAllUsesWith(GEPNew); + GEP1->eraseFromParent(); + } } /// diff --git a/llvm/test/Transforms/MergedLoadStoreMotion/st_sink_no_gep.ll b/llvm/test/Transforms/MergedLoadStoreMotion/st_sink_no_gep.ll index 854ba9e549de..457b4942d812 100644 --- a/llvm/test/Transforms/MergedLoadStoreMotion/st_sink_no_gep.ll +++ b/llvm/test/Transforms/MergedLoadStoreMotion/st_sink_no_gep.ll @@ -7,12 +7,12 @@ define void @no_gep_same_ptr(i1 %c, ptr %p, i32 %x, i32 %y) { ; CHECK-NEXT: entry: ; CHECK-NEXT: br i1 [[C:%.*]], label [[IF_THEN:%.*]], label [[IF_ELSE:%.*]] ; CHECK: if.then: -; CHECK-NEXT: store i32 [[X:%.*]], ptr [[P:%.*]], align 4 ; CHECK-NEXT: br label [[IF_END:%.*]] ; CHECK: if.else: -; CHECK-NEXT: store i32 [[Y:%.*]], ptr [[P]], align 4 ; CHECK-NEXT: br label [[IF_END]] ; CHECK: if.end: +; CHECK-NEXT: [[Y_SINK:%.*]] = phi i32 [ [[X:%.*]], [[IF_THEN]] ], [ [[Y:%.*]], [[IF_ELSE]] ] +; CHECK-NEXT: store i32 [[Y_SINK]], ptr [[P:%.*]], align 4 ; CHECK-NEXT: ret void ; entry: @@ -64,12 +64,12 @@ define void @shared_gep(i1 %c, ptr %p, i32 %x, i32 %y) { ; CHECK-NEXT: [[GEP:%.*]] = getelementptr i32, ptr [[P:%.*]], i32 1 ; CHECK-NEXT: br i1 [[C:%.*]], label [[IF_THEN:%.*]], label [[IF_ELSE:%.*]] ; CHECK: if.then: -; CHECK-NEXT: store i32 [[X:%.*]], ptr [[GEP]], align 4 ; CHECK-NEXT: br label [[IF_END:%.*]] ; CHECK: if.else: -; CHECK-NEXT: store i32 [[Y:%.*]], ptr [[GEP]], align 4 ; CHECK-NEXT: br label [[IF_END]] ; CHECK: if.end: +; CHECK-NEXT: [[Y_SINK:%.*]] = phi i32 [ [[X:%.*]], [[IF_THEN]] ], [ [[Y:%.*]], [[IF_ELSE]] ] +; CHECK-NEXT: store i32 [[Y_SINK]], ptr [[GEP]], align 4 ; CHECK-NEXT: ret void ; entry: diff --git a/llvm/test/Transforms/MergedLoadStoreMotion/st_sink_split_bb.ll b/llvm/test/Transforms/MergedLoadStoreMotion/st_sink_split_bb.ll index b7a022996f3f..55656a8ae0f0 100644 --- a/llvm/test/Transforms/MergedLoadStoreMotion/st_sink_split_bb.ll +++ b/llvm/test/Transforms/MergedLoadStoreMotion/st_sink_split_bb.ll @@ -48,12 +48,12 @@ define dso_local void @st_sink_split_bb(ptr nocapture %arg, ptr nocapture %arg1, ; CHECK-YES: bb5: ; CHECK-YES-NEXT: br i1 [[ARG3:%.*]], label [[BB6:%.*]], label [[BB7:%.*]] ; CHECK-YES: bb6: -; CHECK-YES-NEXT: store i32 2, ptr [[ARG]], align 4 ; CHECK-YES-NEXT: br label [[BB9_SINK_SPLIT:%.*]] ; CHECK-YES: bb7: -; CHECK-YES-NEXT: store i32 3, ptr [[ARG]], align 4 ; CHECK-YES-NEXT: br label [[BB9_SINK_SPLIT]] ; CHECK-YES: bb9.sink.split: +; CHECK-YES-NEXT: [[DOTSINK:%.*]] = phi i32 [ 2, [[BB6]] ], [ 3, [[BB7]] ] +; CHECK-YES-NEXT: store i32 [[DOTSINK]], ptr [[ARG]], align 4 ; CHECK-YES-NEXT: [[TMP0:%.*]] = getelementptr inbounds i32, ptr [[ARG1:%.*]], i64 1 ; CHECK-YES-NEXT: store i32 3, ptr [[TMP0]], align 4 ; CHECK-YES-NEXT: [[TMP1:%.*]] = getelementptr inbounds i32, ptr [[ARG1]], i64 2