[llvm] r270828 - [MergedLoadStoreMotion] Don't transform across may-throw calls

Daniel Berlin via llvm-commits llvm-commits at lists.llvm.org
Thu May 26 16:17:36 PDT 2016


This seems ... problematic, in a simple sense.

You've added a check that literally every instruction in between does not
throw :)

It happens that the alias check is O(N) for memdep so it was all N^2
anyway, but for memoryssa it's O(1), and you've now made such a walk N^2 in
those case.
This whole check should be in the outer loop of mergeLoads and mergeStores.

Instead of just a check of each inst that is a load, check if they are
mayThrow, and if so, stop, because you can't merge any loads (or sink any
stores) from that block.


---------- Forwarded message ----------
From: David Majnemer via llvm-commits <llvm-commits at lists.llvm.org>
Date: Thu, May 26, 2016 at 12:11 AM
Subject: [llvm] r270828 - [MergedLoadStoreMotion] Don't transform across
may-throw calls
To: llvm-commits at lists.llvm.org


Author: majnemer
Date: Thu May 26 02:11:09 2016
New Revision: 270828

URL: http://llvm.org/viewvc/llvm-project?rev=270828&view=rev
Log:
[MergedLoadStoreMotion] Don't transform across may-throw calls

It is unsafe to hoist a load before a function call which may throw, the
throw might prevent a pointer dereference.

Likewise, it is unsafe to sink a store after a call which may throw.
The caller might be able to observe the difference.

This fixes PR27858.

Added:
    llvm/trunk/test/Transforms/InstMerge/exceptions.ll
Modified:
    llvm/trunk/lib/Transforms/Scalar/MergedLoadStoreMotion.cpp
    llvm/trunk/test/Transforms/InstMerge/st_sink_no_barrier_call.ll

Modified: llvm/trunk/lib/Transforms/Scalar/MergedLoadStoreMotion.cpp
URL:
http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/MergedLoadStoreMotion.cpp?rev=270828&r1=270827&r2=270828&view=diff
==============================================================================
--- llvm/trunk/lib/Transforms/Scalar/MergedLoadStoreMotion.cpp (original)
+++ llvm/trunk/lib/Transforms/Scalar/MergedLoadStoreMotion.cpp Thu May 26
02:11:09 2016
@@ -79,7 +79,6 @@
 #include "llvm/Analysis/Loads.h"
 #include "llvm/Analysis/MemoryBuiltins.h"
 #include "llvm/Analysis/MemoryDependenceAnalysis.h"
-#include "llvm/Analysis/TargetLibraryInfo.h"
 #include "llvm/IR/Metadata.h"
 #include "llvm/IR/PatternMatch.h"
 #include "llvm/Support/Debug.h"
@@ -114,7 +113,6 @@ private:
   // This transformation requires dominator postdominator info
   void getAnalysisUsage(AnalysisUsage &AU) const override {
     AU.setPreservesCFG();
-    AU.addRequired<TargetLibraryInfoWrapperPass>();
     AU.addRequired<AAResultsWrapperPass>();
     AU.addPreserved<GlobalsAAWrapperPass>();
     AU.addPreserved<MemoryDependenceWrapperPass>();
@@ -130,9 +128,9 @@ private:
   BasicBlock *getDiamondTail(BasicBlock *BB);
   bool isDiamondHead(BasicBlock *BB);
   // Routines for hoisting loads
-  bool isLoadHoistBarrierInRange(const Instruction& Start,
-                                 const Instruction& End,
-                                 LoadInst* LI);
+  bool isLoadHoistBarrierInRange(const Instruction &Start,
+                                 const Instruction &End, LoadInst *LI,
+                                 bool SafeToLoadUnconditionally);
   LoadInst *canHoistFromBlock(BasicBlock *BB, LoadInst *LI);
   void hoistInstruction(BasicBlock *BB, Instruction *HoistCand,
                         Instruction *ElseInst);
@@ -166,7 +164,6 @@ FunctionPass *llvm::createMergedLoadStor
 INITIALIZE_PASS_BEGIN(MergedLoadStoreMotion, "mldst-motion",
                       "MergedLoadStoreMotion", false, false)
 INITIALIZE_PASS_DEPENDENCY(MemoryDependenceWrapperPass)
-INITIALIZE_PASS_DEPENDENCY(TargetLibraryInfoWrapperPass)
 INITIALIZE_PASS_DEPENDENCY(AAResultsWrapperPass)
 INITIALIZE_PASS_DEPENDENCY(GlobalsAAWrapperPass)
 INITIALIZE_PASS_END(MergedLoadStoreMotion, "mldst-motion",
@@ -229,9 +226,14 @@ bool MergedLoadStoreMotion::isDiamondHea
 /// being loaded or protect against the load from happening
 /// it is considered a hoist barrier.
 ///
-bool MergedLoadStoreMotion::isLoadHoistBarrierInRange(const Instruction&
Start,
-                                                      const Instruction&
End,
-                                                      LoadInst* LI) {
+bool MergedLoadStoreMotion::isLoadHoistBarrierInRange(
+    const Instruction &Start, const Instruction &End, LoadInst *LI,
+    bool SafeToLoadUnconditionally) {
+  if (!SafeToLoadUnconditionally)
+    for (const Instruction &Inst :
+         make_range(Start.getIterator(), End.getIterator()))
+      if (Inst.mayThrow())
+        return true;
   MemoryLocation Loc = MemoryLocation::get(LI);
   return AA->canInstructionRangeModRef(Start, End, Loc, MRI_Mod);
 }
@@ -245,23 +247,28 @@ bool MergedLoadStoreMotion::isLoadHoistB
 ///
 LoadInst *MergedLoadStoreMotion::canHoistFromBlock(BasicBlock *BB1,
                                                    LoadInst *Load0) {
-
+  BasicBlock *BB0 = Load0->getParent();
+  BasicBlock *Head = BB0->getSinglePredecessor();
+  bool SafeToLoadUnconditionally = isSafeToLoadUnconditionally(
+      Load0->getPointerOperand(), Load0->getAlignment(),
+      Load0->getModule()->getDataLayout(),
+      /*ScanFrom=*/Head->getTerminator());
   for (BasicBlock::iterator BBI = BB1->begin(), BBE = BB1->end(); BBI !=
BBE;
        ++BBI) {
     Instruction *Inst = &*BBI;

     // Only merge and hoist loads when their result in used only in BB
-    if (!isa<LoadInst>(Inst) || Inst->isUsedOutsideOfBlock(BB1))
+    auto *Load1 = dyn_cast<LoadInst>(Inst);
+    if (!Load1 || Inst->isUsedOutsideOfBlock(BB1))
       continue;

-    auto *Load1 = cast<LoadInst>(Inst);
-    BasicBlock *BB0 = Load0->getParent();
-
     MemoryLocation Loc0 = MemoryLocation::get(Load0);
     MemoryLocation Loc1 = MemoryLocation::get(Load1);
     if (AA->isMustAlias(Loc0, Loc1) && Load0->isSameOperationAs(Load1) &&
-        !isLoadHoistBarrierInRange(BB1->front(), *Load1, Load1) &&
-        !isLoadHoistBarrierInRange(BB0->front(), *Load0, Load0)) {
+        !isLoadHoistBarrierInRange(BB1->front(), *Load1, Load1,
+                                   SafeToLoadUnconditionally) &&
+        !isLoadHoistBarrierInRange(BB0->front(), *Load0, Load0,
+                                   SafeToLoadUnconditionally)) {
       return Load1;
     }
   }
@@ -387,6 +394,10 @@ bool MergedLoadStoreMotion::mergeLoads(B
 bool MergedLoadStoreMotion::isStoreSinkBarrierInRange(const Instruction
&Start,
                                                       const Instruction
&End,
                                                       MemoryLocation Loc) {
+  for (const Instruction &Inst :
+       make_range(Start.getIterator(), End.getIterator()))
+    if (Inst.mayThrow())
+      return true;
   return AA->canInstructionRangeModRef(Start, End, Loc, MRI_ModRef);
 }

@@ -403,18 +414,15 @@ StoreInst *MergedLoadStoreMotion::canSin
        RBI != RBE; ++RBI) {
     Instruction *Inst = &*RBI;

-    if (!isa<StoreInst>(Inst))
-       continue;
-
-    StoreInst *Store1 = cast<StoreInst>(Inst);
+    auto *Store1 = dyn_cast<StoreInst>(Inst);
+    if (!Store1)
+      continue;

     MemoryLocation Loc0 = MemoryLocation::get(Store0);
     MemoryLocation Loc1 = MemoryLocation::get(Store1);
     if (AA->isMustAlias(Loc0, Loc1) && Store0->isSameOperationAs(Store1) &&
-
!isStoreSinkBarrierInRange(*(std::next(BasicBlock::iterator(Store1))),
-                                   BB1->back(), Loc1) &&
-
!isStoreSinkBarrierInRange(*(std::next(BasicBlock::iterator(Store0))),
-                                   BB0->back(), Loc0)) {
+        !isStoreSinkBarrierInRange(*Store1->getNextNode(), BB1->back(),
Loc1) &&
+        !isStoreSinkBarrierInRange(*Store0->getNextNode(), BB0->back(),
Loc0)) {
       return Store1;
     }
   }

Added: llvm/trunk/test/Transforms/InstMerge/exceptions.ll
URL:
http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstMerge/exceptions.ll?rev=270828&view=auto
==============================================================================
--- llvm/trunk/test/Transforms/InstMerge/exceptions.ll (added)
+++ llvm/trunk/test/Transforms/InstMerge/exceptions.ll Thu May 26 02:11:09
2016
@@ -0,0 +1,57 @@
+; RUN: opt -basicaa -memdep -mldst-motion -S < %s | FileCheck %s
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+ at r = common global i32 0, align 4
+ at s = common global i32 0, align 4
+
+; CHECK-LABEL: define void @test1(
+define void @test1(i1 %cmp, i32* noalias %p) {
+entry:
+  br i1 %cmp, label %if.then, label %if.else
+
+if.then:                                          ; preds = %entry
+  call void @may_throw()
+  %arrayidx = getelementptr inbounds i32, i32* %p, i64 1
+  %0 = load i32, i32* %arrayidx, align 4
+  store i32 %0, i32* @r, align 4
+  br label %if.end
+; CHECK:       call void @may_throw()
+; CHECK-NEXT:  %[[gep:.*]] = getelementptr inbounds i32, i32* %p, i64 1
+; CHECK-NEXT:  %[[load:.*]] = load i32, i32* %[[gep]], align 4
+; CHECK-NEXT:  store i32 %[[load]], i32* @r, align 4
+
+if.else:                                          ; preds = %entry
+  %arrayidx1 = getelementptr inbounds i32, i32* %p, i64 1
+  %1 = load i32, i32* %arrayidx1, align 4
+  store i32 %1, i32* @s, align 4
+  br label %if.end
+
+if.end:                                           ; preds = %if.else,
%if.then
+  ret void
+}
+
+; CHECK-LABEL: define void @test2(
+define void @test2(i1 %cmp, i32* noalias %p) {
+entry:
+  br i1 %cmp, label %if.then, label %if.else
+
+if.then:                                          ; preds = %entry
+  %arrayidx = getelementptr inbounds i32, i32* %p, i64 1
+  store i32 1, i32* %arrayidx, align 4
+  call void @may_throw()
+; CHECK:       %[[gep:.*]] = getelementptr inbounds i32, i32* %p, i64 1
+; CHECK-NEXT:  store i32 1, i32* %[[gep]], align 4
+; CHECK-NEXT:  call void @may_throw()
+  br label %if.end
+
+if.else:                                          ; preds = %entry
+  %arrayidx1 = getelementptr inbounds i32, i32* %p, i64 1
+  store i32 2, i32* %arrayidx1, align 4
+  br label %if.end
+
+if.end:                                           ; preds = %if.else,
%if.then
+  ret void
+}
+
+declare void @may_throw()

Modified: llvm/trunk/test/Transforms/InstMerge/st_sink_no_barrier_call.ll
URL:
http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstMerge/st_sink_no_barrier_call.ll?rev=270828&r1=270827&r2=270828&view=diff
==============================================================================
--- llvm/trunk/test/Transforms/InstMerge/st_sink_no_barrier_call.ll
(original)
+++ llvm/trunk/test/Transforms/InstMerge/st_sink_no_barrier_call.ll Thu May
26 02:11:09 2016
@@ -33,7 +33,7 @@ if.else:
   %p3 = getelementptr inbounds %struct.node, %struct.node* %node.017, i32
0, i32 6
   ; CHECK-NOT: store i32
   store i32 %add, i32* %p3, align 4
-  call i32 @foo(i32 5)                           ;not a barrier
+  call i32 @foo(i32 5) nounwind                                  ;not a
barrier
   br label %if.end

 ; CHECK: if.end


_______________________________________________
llvm-commits mailing list
llvm-commits at lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160526/bcaca0a0/attachment.html>


More information about the llvm-commits mailing list