From 42c3f554f4d5e505c3e43cdcf722ec82a7a3f6a5 Mon Sep 17 00:00:00 2001 From: Dale Johannesen Date: Wed, 17 Jun 2009 20:48:23 +0000 Subject: [PATCH] This fixes a bug introduced in 72661, which can move loads back past a check that the load address is valid, see new testcase. The test that went in with 72661 has exactly this case, except that the conditional it's moving past is checking something else; I've settled for changing that test to reference a global, not a pointer. It may be possible to scan all the tests you pass and make sure none of them are checking any component of the address, but it's not trivial and I'm not trying to do that here. git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@73632 91177308-0d34-0410-b5e6-96231b3b80d8 --- lib/Transforms/Scalar/GVN.cpp | 19 +++++- test/Transforms/GVN/2009-06-17-InvalidPRE.ll | 72 ++++++++++++++++++++ test/Transforms/GVN/pre-single-pred.ll | 9 +-- 3 files changed, 95 insertions(+), 5 deletions(-) create mode 100644 test/Transforms/GVN/2009-06-17-InvalidPRE.ll diff --git a/lib/Transforms/Scalar/GVN.cpp b/lib/Transforms/Scalar/GVN.cpp index 1e89ef7b22d..f4a98984447 100644 --- a/lib/Transforms/Scalar/GVN.cpp +++ b/lib/Transforms/Scalar/GVN.cpp @@ -37,6 +37,7 @@ #include "llvm/Support/Compiler.h" #include "llvm/Support/Debug.h" #include "llvm/Transforms/Utils/BasicBlockUtils.h" +#include "llvm/Transforms/Utils/Local.h" #include using namespace llvm; @@ -1075,6 +1076,7 @@ bool GVN::processNonLocalLoad(LoadInst *LI, BasicBlock *TmpBB = LoadBB; bool isSinglePred = false; + bool allSingleSucc = true; while (TmpBB->getSinglePredecessor()) { isSinglePred = true; TmpBB = TmpBB->getSinglePredecessor(); @@ -1084,6 +1086,8 @@ bool GVN::processNonLocalLoad(LoadInst *LI, return false; if (Blockers.count(TmpBB)) return false; + if (TmpBB->getTerminator()->getNumSuccessors() != 1) + allSingleSucc = false; } assert(TmpBB); @@ -1160,7 +1164,20 @@ bool GVN::processNonLocalLoad(LoadInst *LI, << UnavailablePred->getName() << "': " << *LI); return false; } - + + // Make sure it is valid to move this load here. We have to watch out for: + // @1 = getelementptr (i8* p, ... + // test p and branch if == 0 + // load @1 + // It is valid to have the getelementptr before the test, even if p can be 0, + // as getelementptr only does address arithmetic. + // If we are not pushing the value through any multiple-successor blocks + // we do not have this case. Otherwise, check that the load is safe to + // put anywhere; this can be improved, but should be conservatively safe. + if (!allSingleSucc && + !isSafeToLoadUnconditionally(LoadPtr, UnavailablePred->getTerminator())) + return false; + // Okay, we can eliminate this load by inserting a reload in the predecessor // and using PHI construction to get the value in the other predecessors, do // it. diff --git a/test/Transforms/GVN/2009-06-17-InvalidPRE.ll b/test/Transforms/GVN/2009-06-17-InvalidPRE.ll new file mode 100644 index 00000000000..c8982c86cb9 --- /dev/null +++ b/test/Transforms/GVN/2009-06-17-InvalidPRE.ll @@ -0,0 +1,72 @@ +; RUN: llvm-as < %s | opt -gvn -enable-load-pre | llvm-dis | not grep pre1 +; GVN load pre was hoisting the loads at %13 and %16 up to bb4.outer. +; This is invalid as it bypasses the check for %m.0.ph==null in bb4. +; ModuleID = 'mbuf.c' +target datalayout = "e-p:32:32:32-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:32:64-f32:32:32-f64:32:64-v64:64:64-v128:128:128-a0:0:64-f80:128:128" +target triple = "i386-apple-darwin9.6" + %struct.mbuf = type { %struct.mbuf*, %struct.mbuf*, i32, i8*, i16, i16, i32 } + +define void @m_adj(%struct.mbuf* %mp, i32 %req_len) nounwind optsize { +entry: + %0 = icmp eq %struct.mbuf* %mp, null ; [#uses=1] + %1 = icmp slt i32 %req_len, 0 ; [#uses=1] + %or.cond = or i1 %1, %0 ; [#uses=1] + br i1 %or.cond, label %return, label %bb4.preheader + +bb4.preheader: ; preds = %entry + br label %bb4.outer + +bb2: ; preds = %bb1 + %2 = sub i32 %len.0, %13 ; [#uses=1] + %3 = getelementptr %struct.mbuf* %m.0.ph, i32 0, i32 2 ; [#uses=1] + store i32 0, i32* %3, align 4 + %4 = getelementptr %struct.mbuf* %m.0.ph, i32 0, i32 0 ; <%struct.mbuf**> [#uses=1] + %5 = load %struct.mbuf** %4, align 4 ; <%struct.mbuf*> [#uses=1] + br label %bb4.outer + +bb4.outer: ; preds = %bb4.preheader, %bb2 + %m.0.ph = phi %struct.mbuf* [ %5, %bb2 ], [ %mp, %bb4.preheader ] ; <%struct.mbuf*> [#uses=7] + %len.0.ph = phi i32 [ %2, %bb2 ], [ %req_len, %bb4.preheader ] ; [#uses=1] + %6 = icmp ne %struct.mbuf* %m.0.ph, null ; [#uses=1] + %7 = getelementptr %struct.mbuf* %m.0.ph, i32 0, i32 2 ; [#uses=1] + %8 = getelementptr %struct.mbuf* %m.0.ph, i32 0, i32 2 ; [#uses=1] + %9 = getelementptr %struct.mbuf* %m.0.ph, i32 0, i32 3 ; [#uses=1] + %10 = getelementptr %struct.mbuf* %m.0.ph, i32 0, i32 3 ; [#uses=1] + br label %bb4 + +bb4: ; preds = %bb4.outer, %bb3 + %len.0 = phi i32 [ 0, %bb3 ], [ %len.0.ph, %bb4.outer ] ; [#uses=6] + %11 = icmp sgt i32 %len.0, 0 ; [#uses=1] + %12 = and i1 %11, %6 ; [#uses=1] + br i1 %12, label %bb1, label %bb7 + +bb1: ; preds = %bb4 + %13 = load i32* %7, align 4 ; [#uses=3] + %14 = icmp sgt i32 %13, %len.0 ; [#uses=1] + br i1 %14, label %bb3, label %bb2 + +bb3: ; preds = %bb1 + %15 = sub i32 %13, %len.0 ; [#uses=1] + store i32 %15, i32* %8, align 4 + %16 = load i8** %9, align 4 ; [#uses=1] + %17 = getelementptr i8* %16, i32 %len.0 ; [#uses=1] + store i8* %17, i8** %10, align 4 + br label %bb4 + +bb7: ; preds = %bb4 + %18 = getelementptr %struct.mbuf* %mp, i32 0, i32 5 ; [#uses=1] + %19 = load i16* %18, align 2 ; [#uses=1] + %20 = zext i16 %19 to i32 ; [#uses=1] + %21 = and i32 %20, 2 ; [#uses=1] + %22 = icmp eq i32 %21, 0 ; [#uses=1] + br i1 %22, label %return, label %bb8 + +bb8: ; preds = %bb7 + %23 = sub i32 %req_len, %len.0 ; [#uses=1] + %24 = getelementptr %struct.mbuf* %mp, i32 0, i32 6 ; [#uses=1] + store i32 %23, i32* %24, align 4 + ret void + +return: ; preds = %bb7, %entry + ret void +} diff --git a/test/Transforms/GVN/pre-single-pred.ll b/test/Transforms/GVN/pre-single-pred.ll index b735ea9827c..cb71617caed 100644 --- a/test/Transforms/GVN/pre-single-pred.ll +++ b/test/Transforms/GVN/pre-single-pred.ll @@ -1,6 +1,7 @@ ; RUN: llvm-as < %s | opt -gvn -enable-load-pre | llvm-dis | not grep {tmp3 = load} -define i32 @f(i32* nocapture %p, i32 %n) nounwind { +@p = external global i32 +define i32 @f(i32 %n) nounwind { entry: br label %for.cond @@ -13,9 +14,9 @@ for.cond.for.end_crit_edge: ; preds = %for.cond br label %for.end for.body: ; preds = %for.cond - %tmp3 = load i32* %p ; [#uses=1] + %tmp3 = load i32* @p ; [#uses=1] %dec = add i32 %tmp3, -1 ; [#uses=2] - store i32 %dec, i32* %p + store i32 %dec, i32* @p %cmp6 = icmp slt i32 %dec, 0 ; [#uses=1] br i1 %cmp6, label %for.body.for.end_crit_edge, label %for.inc @@ -27,6 +28,6 @@ for.inc: ; preds = %for.body br label %for.cond for.end: ; preds = %for.body.for.end_crit_edge, %for.cond.for.end_crit_edge - %tmp9 = load i32* %p ; [#uses=1] + %tmp9 = load i32* @p ; [#uses=1] ret i32 %tmp9 }