[llvm] r315440 - [GVN] Prevent LoadPRE from hoisting across instructions that don't pass control flow to successors
Max Kazantsev via llvm-commits
llvm-commits at lists.llvm.org
Wed Oct 11 01:10:44 PDT 2017
Author: mkazantsev
Date: Wed Oct 11 01:10:43 2017
New Revision: 315440
URL: http://llvm.org/viewvc/llvm-project?rev=315440&view=rev
Log:
[GVN] Prevent LoadPRE from hoisting across instructions that don't pass control flow to successors
This patch fixes the miscompile that happens when PRE hoists loads across guards and
other instructions that don't always pass control flow to their successors. PRE is now prohibited
to hoist across such instructions because there is no guarantee that the load standing after such
instruction is still valid before such instruction. For example, a load from under a guard may be
invalid before the guard in the following case:
int array[LEN];
...
guard(0 <= index && index < LEN);
use(array[index]);
Differential Revision: https://reviews.llvm.org/D37460
Added:
llvm/trunk/test/Transforms/GVN/PRE/pre-load-guards.ll
Modified:
llvm/trunk/include/llvm/Transforms/Scalar/GVN.h
llvm/trunk/lib/Transforms/Scalar/GVN.cpp
llvm/trunk/test/Transforms/GVN/PRE/pre-load.ll
Modified: llvm/trunk/include/llvm/Transforms/Scalar/GVN.h
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Transforms/Scalar/GVN.h?rev=315440&r1=315439&r2=315440&view=diff
==============================================================================
--- llvm/trunk/include/llvm/Transforms/Scalar/GVN.h (original)
+++ llvm/trunk/include/llvm/Transforms/Scalar/GVN.h Wed Oct 11 01:10:43 2017
@@ -18,6 +18,7 @@
#include "llvm/ADT/DenseMap.h"
#include "llvm/ADT/MapVector.h"
+#include "llvm/ADT/PostOrderIterator.h"
#include "llvm/ADT/SetVector.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/Analysis/AliasAnalysis.h"
@@ -27,6 +28,7 @@
#include "llvm/IR/PassManager.h"
#include "llvm/Support/Allocator.h"
#include "llvm/Support/Compiler.h"
+#include "llvm/Transforms/Utils/OrderedInstructions.h"
#include <cstdint>
#include <utility>
#include <vector>
@@ -156,7 +158,11 @@ private:
AssumptionCache *AC;
SetVector<BasicBlock *> DeadBlocks;
OptimizationRemarkEmitter *ORE;
+ // Maps a block to the topmost instruction with implicit control flow in it.
+ DenseMap<const BasicBlock *, const Instruction *>
+ FirstImplicitControlFlowInsts;
+ OrderedInstructions *OI;
ValueTable VN;
/// A mapping from value numbers to lists of Value*'s that
@@ -268,6 +274,7 @@ private:
BasicBlock *Curr, unsigned int ValNo);
Value *findLeader(const BasicBlock *BB, uint32_t num);
void cleanupGlobalSets();
+ void fillImplicitControlFlowInfo(ReversePostOrderTraversal<Function *> &RPOT);
void verifyRemoved(const Instruction *I) const;
bool splitCriticalEdges();
BasicBlock *splitCriticalEdges(BasicBlock *Pred, BasicBlock *Succ);
Modified: llvm/trunk/lib/Transforms/Scalar/GVN.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Scalar/GVN.cpp?rev=315440&r1=315439&r2=315440&view=diff
==============================================================================
--- llvm/trunk/lib/Transforms/Scalar/GVN.cpp (original)
+++ llvm/trunk/lib/Transforms/Scalar/GVN.cpp Wed Oct 11 01:10:43 2017
@@ -38,6 +38,7 @@
#include "llvm/Analysis/OptimizationRemarkEmitter.h"
#include "llvm/Analysis/PHITransAddr.h"
#include "llvm/Analysis/TargetLibraryInfo.h"
+#include "llvm/Analysis/ValueTracking.h"
#include "llvm/IR/Attributes.h"
#include "llvm/IR/BasicBlock.h"
#include "llvm/IR/CallSite.h"
@@ -1048,7 +1049,32 @@ bool GVN::PerformLoadPRE(LoadInst *LI, A
// backwards through predecessors if needed.
BasicBlock *LoadBB = LI->getParent();
BasicBlock *TmpBB = LoadBB;
+ bool IsSafeToSpeculativelyExecute = isSafeToSpeculativelyExecute(LI);
+ // Check that there is no implicit control flow instructions above our load in
+ // its block. If there is an instruction that doesn't always pass the
+ // execution to the following instruction, then moving through it may become
+ // invalid. For example:
+ //
+ // int arr[LEN];
+ // int index = ???;
+ // ...
+ // guard(0 <= index && index < LEN);
+ // use(arr[index]);
+ //
+ // It is illegal to move the array access to any point above the guard,
+ // because if the index is out of bounds we should deoptimize rather than
+ // access the array.
+ // Check that there is no guard in this block above our intruction.
+ if (!IsSafeToSpeculativelyExecute) {
+ auto It = FirstImplicitControlFlowInsts.find(TmpBB);
+ if (It != FirstImplicitControlFlowInsts.end()) {
+ assert(It->second->getParent() == TmpBB &&
+ "Implicit control flow map broken?");
+ if (OI->dominates(It->second, LI))
+ return false;
+ }
+ }
while (TmpBB->getSinglePredecessor()) {
TmpBB = TmpBB->getSinglePredecessor();
if (TmpBB == LoadBB) // Infinite (unreachable) loop.
@@ -1063,6 +1089,11 @@ bool GVN::PerformLoadPRE(LoadInst *LI, A
// which it was not previously executed.
if (TmpBB->getTerminator()->getNumSuccessors() != 1)
return false;
+
+ // Check that there is no implicit control flow in a block above.
+ if (!IsSafeToSpeculativelyExecute &&
+ FirstImplicitControlFlowInsts.count(TmpBB))
+ return false;
}
assert(TmpBB);
@@ -1979,6 +2010,8 @@ bool GVN::runImpl(Function &F, Assumptio
TLI = &RunTLI;
VN.setAliasAnalysis(&RunAA);
MD = RunMD;
+ OrderedInstructions OrderedInstrs(DT);
+ OI = &OrderedInstrs;
VN.setMemDep(MD);
ORE = RunORE;
@@ -2068,6 +2101,9 @@ bool GVN::processBlock(BasicBlock *BB) {
DEBUG(verifyRemoved(*I));
(*I)->eraseFromParent();
}
+
+ if (!InstrsToErase.empty())
+ OI->invalidateBlock(BB);
InstrsToErase.clear();
if (AtStart)
@@ -2263,6 +2299,7 @@ bool GVN::performScalarPRE(Instruction *
MD->removeInstruction(CurInst);
DEBUG(verifyRemoved(CurInst));
CurInst->eraseFromParent();
+ OI->invalidateBlock(CurrentBlock);
++NumGVNInstr;
return true;
@@ -2329,6 +2366,7 @@ bool GVN::iterateOnFunction(Function &F)
// RPOT walks the graph in its constructor and will not be invalidated during
// processBlock.
ReversePostOrderTraversal<Function *> RPOT(&F);
+ fillImplicitControlFlowInfo(RPOT);
for (BasicBlock *BB : RPOT)
Changed |= processBlock(BB);
@@ -2340,6 +2378,45 @@ void GVN::cleanupGlobalSets() {
LeaderTable.clear();
BlockRPONumber.clear();
TableAllocator.Reset();
+ FirstImplicitControlFlowInsts.clear();
+}
+
+void
+GVN::fillImplicitControlFlowInfo(ReversePostOrderTraversal<Function *> &RPOT) {
+ auto MayNotTransferExecutionToSuccessor = [&](const Instruction *I) {
+ // If a block's instruction doesn't always pass the control to its successor
+ // instruction, mark the block as having implicit control flow. We use them
+ // to avoid wrong assumptions of sort "if A is executed and B post-dominates
+ // A, then B is also executed". This is not true is there is an implicit
+ // control flow instruction (e.g. a guard) between them.
+ //
+ // TODO: Currently, isGuaranteedToTransferExecutionToSuccessor returns false
+ // for volatile stores and loads because they can trap. The discussion on
+ // whether or not it is correct is still ongoing. We might want to get rid
+ // of this logic in the future. Anyways, trapping instructions shouldn't
+ // introduce implicit control flow, so we explicitly allow them here. This
+ // must be removed once isGuaranteedToTransferExecutionToSuccessor is fixed.
+ if (isGuaranteedToTransferExecutionToSuccessor(I))
+ return false;
+ if (auto *LI = dyn_cast<LoadInst>(I)) {
+ assert(LI->isVolatile() && "Non-volatile load should transfer execution"
+ " to successor!");
+ return false;
+ }
+ if (auto *SI = dyn_cast<StoreInst>(I)) {
+ assert(SI->isVolatile() && "Non-volatile store should transfer execution"
+ " to successor!");
+ return false;
+ }
+ return true;
+ };
+
+ for (BasicBlock *BB : RPOT)
+ for (auto &I : *BB)
+ if (MayNotTransferExecutionToSuccessor(&I)) {
+ FirstImplicitControlFlowInsts[BB] = &I;
+ break;
+ }
}
/// Verify that the specified instruction does not occur in our
Added: llvm/trunk/test/Transforms/GVN/PRE/pre-load-guards.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/GVN/PRE/pre-load-guards.ll?rev=315440&view=auto
==============================================================================
--- llvm/trunk/test/Transforms/GVN/PRE/pre-load-guards.ll (added)
+++ llvm/trunk/test/Transforms/GVN/PRE/pre-load-guards.ll Wed Oct 11 01:10:43 2017
@@ -0,0 +1,146 @@
+; RUN: opt < %s -basicaa -gvn -enable-load-pre -S | FileCheck %s
+target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64"
+
+declare void @llvm.experimental.guard(i1, ...)
+
+; This is a motivating example on why we prohibit hoisting through guards.
+; In the bottom block, we check that the index is within bounds and only access
+; the element in this case and deoptimize otherwise. If we hoist the load to a
+; place above the guard, it will may lead to out-of-bound array access.
+define i32 @test_motivation(i32* %p, i32* %q, i1 %C, i32 %index, i32 %len) {
+; CHECK-LABEL: @test_motivation(
+block1:
+ %el1 = getelementptr inbounds i32, i32* %q, i32 %index
+ %el2 = getelementptr inbounds i32, i32* %p, i32 %index
+ br i1 %C, label %block2, label %block3
+
+block2:
+
+; CHECK: block2:
+; CHECK-NEXT: br
+; CHECK-NOT: load
+; CHECK-NOT: sge
+; CHECK-NOT: slt
+; CHECK-NOT: and
+ br label %block4
+
+block3:
+ store i32 0, i32* %el1
+ br label %block4
+
+block4:
+
+; CHECK: block4:
+; CHECK: %cond1 = icmp sge i32 %index, 0
+; CHECK-NEXT: %cond2 = icmp slt i32 %index, %len
+; CHECK-NEXT: %in.bounds = and i1 %cond1, %cond2
+; CHECK: call void (i1, ...) @llvm.experimental.guard(i1 %in.bounds)
+; CHECK-NEXT: %PRE = load i32, i32* %P2
+; CHECK: ret i32 %PRE
+
+ %P2 = phi i32* [%el2, %block3], [%el1, %block2]
+ %cond1 = icmp sge i32 %index, 0
+ %cond2 = icmp slt i32 %index, %len
+ %in.bounds = and i1 %cond1, %cond2
+ call void (i1, ...) @llvm.experimental.guard(i1 %in.bounds) [ "deopt"() ]
+ %PRE = load i32, i32* %P2
+ ret i32 %PRE
+}
+
+; Guard in load's block that is above the load should prohibit the PRE.
+define i32 @test_guard_01(i32* %p, i32* %q, i1 %C, i1 %G) {
+; CHECK-LABEL: @test_guard_01(
+block1:
+ br i1 %C, label %block2, label %block3
+
+block2:
+
+; CHECK: block2:
+; CHECK-NEXT: br
+; CHECK-NOT: load
+
+ br label %block4
+
+block3:
+ store i32 0, i32* %p
+ br label %block4
+
+block4:
+
+; CHECK: block4:
+; CHECK: call void (i1, ...) @llvm.experimental.guard(i1 %G)
+; CHECK-NEXT: load
+; CHECK: ret i32
+
+ %P2 = phi i32* [%p, %block3], [%q, %block2]
+ call void (i1, ...) @llvm.experimental.guard(i1 %G) [ "deopt"() ]
+ %PRE = load i32, i32* %P2
+ ret i32 %PRE
+}
+
+; Guard in load's block that is below the load should not prohibit the PRE.
+define i32 @test_guard_02(i32* %p, i32* %q, i1 %C, i1 %G) {
+; CHECK-LABEL: @test_guard_02(
+block1:
+ br i1 %C, label %block2, label %block3
+
+block2:
+
+; CHECK: block2:
+; CHECK-NEXT: load i32, i32* %q
+
+ br label %block4
+
+block3:
+ store i32 0, i32* %p
+ br label %block4
+
+block4:
+
+; CHECK: block4:
+; CHECK-NEXT: phi i32 [
+; CHECK-NEXT: phi i32* [
+; CHECK-NEXT: call void (i1, ...) @llvm.experimental.guard(i1 %G)
+; CHECK-NOT: load
+; CHECK: ret i32
+
+ %P2 = phi i32* [%p, %block3], [%q, %block2]
+ %PRE = load i32, i32* %P2
+ call void (i1, ...) @llvm.experimental.guard(i1 %G) [ "deopt"() ]
+ ret i32 %PRE
+}
+
+; Guard above the load's block should prevent PRE from hoisting through it.
+define i32 @test_guard_03(i32* %p, i32* %q, i1 %C, i1 %G) {
+; CHECK-LABEL: @test_guard_03(
+block1:
+ br i1 %C, label %block2, label %block3
+
+block2:
+
+; CHECK: block2:
+; CHECK-NEXT: br
+; CHECK-NOT: load
+
+ br label %block4
+
+block3:
+ store i32 0, i32* %p
+ br label %block4
+
+block4:
+
+; CHECK: block4:
+; CHECK-NEXT: phi i32*
+; CHECK-NEXT: call void (i1, ...) @llvm.experimental.guard(i1 %G)
+; CHECK-NEXT: load
+; CHECK-NEXT: ret i32
+
+ %P2 = phi i32* [%p, %block3], [%q, %block2]
+ call void (i1, ...) @llvm.experimental.guard(i1 %G) [ "deopt"() ]
+ br label %block5
+
+block5:
+ %PRE = load i32, i32* %P2
+ ret i32 %PRE
+}
Modified: llvm/trunk/test/Transforms/GVN/PRE/pre-load.ll
URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/GVN/PRE/pre-load.ll?rev=315440&r1=315439&r2=315440&view=diff
==============================================================================
--- llvm/trunk/test/Transforms/GVN/PRE/pre-load.ll (original)
+++ llvm/trunk/test/Transforms/GVN/PRE/pre-load.ll Wed Oct 11 01:10:43 2017
@@ -430,3 +430,31 @@ cleanup2:
call void @g(i32 %NOTPRE)
cleanupret from %c2 unwind to caller
}
+
+; Don't PRE load across calls.
+
+define i32 @test13(i32* noalias nocapture readonly %x, i32* noalias nocapture %r, i32 %a) {
+; CHECK-LABEL: @test13(
+; CHECK: entry:
+; CHECK-NEXT: icmp eq
+; CHECK-NEXT: br i1
+entry:
+ %tobool = icmp eq i32 %a, 0
+ br i1 %tobool, label %if.end, label %if.then
+
+; CHECK: if.then:
+; CHECK-NEXT: load i32
+; CHECK-NEXT: store i32
+if.then:
+ %uu = load i32, i32* %x, align 4
+ store i32 %uu, i32* %r, align 4
+ br label %if.end
+
+; CHECK: if.end:
+; CHECK-NEXT: call void @f()
+; CHECK-NEXT: load i32
+if.end:
+ call void @f()
+ %vv = load i32, i32* %x, align 4
+ ret i32 %vv
+}
More information about the llvm-commits
mailing list