From 6ea47759008526dc11f5064b266de95c61915581 Mon Sep 17 00:00:00 2001 From: Hans Wennborg Date: Wed, 13 Nov 2019 11:59:22 +0100 Subject: [PATCH] Revert 57dd4b0 "[ValueTracking] Allow context-sensitive nullness check for non-pointers" This caused miscompiles of Chromium (https://crbug.com/1023818). The reduced repro is small enough to fit here: $ cat /tmp/a.c unsigned char f(unsigned char *p) { unsigned char result = 0; for (int shift = 0; shift < 1; ++shift) result |= p[0] << (shift * 8); return result; } $ bin/clang -O2 -S -o - /tmp/a.c | grep -A4 f: f: # @f .cfi_startproc # %bb.0: # %entry xorl %eax, %eax retq That's nicely optimized, but I don't think it's the right result :-) > Same as D60846 but with a fix for the problem encountered there which > was a missing context adjustment in the handling of PHI nodes. > > The test that caused D60846 to be reverted was added in e15ab8f277c7. > > Reviewers: nikic, nlopes, mkazantsev,spatel, dlrobertson, uabelho, hakzsam > > Subscribers: hiraditya, bollu, llvm-commits > > Tags: #llvm > > Differential Revision: https://reviews.llvm.org/D69571 This reverts commit 57dd4b03e4806bbb4760ab6150940150d884df20. --- llvm/lib/Analysis/InstructionSimplify.cpp | 10 ++-------- llvm/lib/Analysis/ValueTracking.cpp | 9 +++++---- llvm/test/Transforms/FunctionAttrs/nonnull.ll | 2 +- llvm/test/Transforms/InstCombine/known-non-zero.ll | 4 ++-- llvm/test/Transforms/InstSimplify/known-non-zero.ll | 10 +++++++--- llvm/test/Transforms/LICM/hoist-mustexec.ll | 4 +++- 6 files changed, 20 insertions(+), 19 deletions(-) diff --git a/llvm/lib/Analysis/InstructionSimplify.cpp b/llvm/lib/Analysis/InstructionSimplify.cpp index 3ef8f9b00509..d997acb365c4 100644 --- a/llvm/lib/Analysis/InstructionSimplify.cpp +++ b/llvm/lib/Analysis/InstructionSimplify.cpp @@ -543,16 +543,10 @@ static Value *ThreadCmpOverPHI(CmpInst::Predicate Pred, Value *LHS, Value *RHS, // Evaluate the BinOp on the incoming phi values. Value *CommonValue = nullptr; - for (unsigned u = 0, e = PI->getNumIncomingValues(); u < e; ++u) { - Value *Incoming = PI->getIncomingValue(u); - Instruction *InTI = PI->getIncomingBlock(u)->getTerminator(); + for (Value *Incoming : PI->incoming_values()) { // If the incoming value is the phi node itself, it can safely be skipped. if (Incoming == PI) continue; - // Change the context instruction to the "edge" that flows into the phi. - // This is important because that is where incoming is actually "evaluated" - // even though it is used later somewhere else. - Value *V = SimplifyCmpInst(Pred, Incoming, RHS, Q.getWithInstruction(InTI), - MaxRecurse); + Value *V = SimplifyCmpInst(Pred, Incoming, RHS, Q, MaxRecurse); // If the operation failed to simplify, or simplified to a different value // to previously, then give up. if (!V || (CommonValue && V != CommonValue)) diff --git a/llvm/lib/Analysis/ValueTracking.cpp b/llvm/lib/Analysis/ValueTracking.cpp index b26c6294dce1..f03a4a6eee45 100644 --- a/llvm/lib/Analysis/ValueTracking.cpp +++ b/llvm/lib/Analysis/ValueTracking.cpp @@ -1902,8 +1902,8 @@ static bool isGEPKnownNonNull(const GEPOperator *GEP, unsigned Depth, static bool isKnownNonNullFromDominatingCondition(const Value *V, const Instruction *CtxI, const DominatorTree *DT) { - if (isa(V)) - return false; + assert(V->getType()->isPointerTy() && "V must be pointer type"); + assert(!isa(V) && "Did not expect ConstantPointerNull"); if (!CtxI || !DT) return false; @@ -2078,11 +2078,12 @@ bool isKnownNonZero(const Value *V, unsigned Depth, const Query &Q) { } } - if (isKnownNonNullFromDominatingCondition(V, Q.CxtI, Q.DT)) - return true; // Check for recursive pointer simplifications. if (V->getType()->isPointerTy()) { + if (isKnownNonNullFromDominatingCondition(V, Q.CxtI, Q.DT)) + return true; + // Look through bitcast operations, GEPs, and int2ptr instructions as they // do not alter the value, or at least not the nullness property of the // value, e.g., int2ptr is allowed to zero/sign extend the value. diff --git a/llvm/test/Transforms/FunctionAttrs/nonnull.ll b/llvm/test/Transforms/FunctionAttrs/nonnull.ll index acdda1df0ae1..42923cee7708 100644 --- a/llvm/test/Transforms/FunctionAttrs/nonnull.ll +++ b/llvm/test/Transforms/FunctionAttrs/nonnull.ll @@ -873,7 +873,7 @@ define void @PR43833_simple(i32* %0, i32 %1) { ; ATTRIBUTOR_NPM-NEXT: ret void ; ATTRIBUTOR_NPM: 8: ; ATTRIBUTOR_NPM-NEXT: [[TMP9:%.*]] = phi i32 [ 1, [[TMP4]] ], [ [[TMP10:%.*]], [[TMP8]] ] -; ATTRIBUTOR_NPM-NEXT: tail call void @sink(i32* nonnull [[TMP6]]) +; ATTRIBUTOR_NPM-NEXT: tail call void @sink(i32* [[TMP6]]) ; ATTRIBUTOR_NPM-NEXT: [[TMP10]] = add nuw nsw i32 [[TMP9]], 1 ; ATTRIBUTOR_NPM-NEXT: [[TMP11:%.*]] = icmp eq i32 [[TMP10]], [[TMP1]] ; ATTRIBUTOR_NPM-NEXT: br i1 [[TMP11]], label [[TMP7]], label [[TMP8]] diff --git a/llvm/test/Transforms/InstCombine/known-non-zero.ll b/llvm/test/Transforms/InstCombine/known-non-zero.ll index 5467db556632..57980e0890c2 100644 --- a/llvm/test/Transforms/InstCombine/known-non-zero.ll +++ b/llvm/test/Transforms/InstCombine/known-non-zero.ll @@ -13,7 +13,7 @@ define i32 @test0(i64 %x) { ; CHECK-NEXT: [[C:%.*]] = icmp eq i64 [[X:%.*]], 0 ; CHECK-NEXT: br i1 [[C]], label [[EXIT:%.*]], label [[NON_ZERO:%.*]] ; CHECK: non_zero: -; CHECK-NEXT: [[CTZ:%.*]] = call i64 @llvm.cttz.i64(i64 [[X]], i1 true), !range !0 +; CHECK-NEXT: [[CTZ:%.*]] = call i64 @llvm.cttz.i64(i64 [[X]], i1 false), !range !0 ; CHECK-NEXT: [[CTZ32:%.*]] = trunc i64 [[CTZ]] to i32 ; CHECK-NEXT: br label [[EXIT]] ; CHECK: exit: @@ -40,7 +40,7 @@ define i32 @test1(i64 %x) { ; CHECK-NEXT: [[C:%.*]] = icmp eq i64 [[X:%.*]], 0 ; CHECK-NEXT: br i1 [[C]], label [[EXIT:%.*]], label [[NON_ZERO:%.*]] ; CHECK: non_zero: -; CHECK-NEXT: [[CTZ:%.*]] = call i64 @llvm.ctlz.i64(i64 [[X]], i1 true), !range !0 +; CHECK-NEXT: [[CTZ:%.*]] = call i64 @llvm.ctlz.i64(i64 [[X]], i1 false), !range !0 ; CHECK-NEXT: [[CTZ32:%.*]] = trunc i64 [[CTZ]] to i32 ; CHECK-NEXT: br label [[EXIT]] ; CHECK: exit: diff --git a/llvm/test/Transforms/InstSimplify/known-non-zero.ll b/llvm/test/Transforms/InstSimplify/known-non-zero.ll index 1a985ede9a43..7e819e82b907 100644 --- a/llvm/test/Transforms/InstSimplify/known-non-zero.ll +++ b/llvm/test/Transforms/InstSimplify/known-non-zero.ll @@ -7,7 +7,8 @@ define i64 @test0(i64 %x) { ; CHECK-NEXT: [[A:%.*]] = icmp eq i64 [[X:%.*]], 0 ; CHECK-NEXT: br i1 [[A]], label [[EXIT:%.*]], label [[NON_ZERO:%.*]] ; CHECK: non_zero: -; CHECK-NEXT: br i1 false, label [[UNREACHABLE:%.*]], label [[EXIT]] +; CHECK-NEXT: [[B:%.*]] = icmp eq i64 [[X]], 0 +; CHECK-NEXT: br i1 [[B]], label [[UNREACHABLE:%.*]], label [[EXIT]] ; CHECK: unreachable: ; CHECK-NEXT: br label [[EXIT]] ; CHECK: exit: @@ -36,7 +37,8 @@ define i64 @test1(i64 %x) { ; CHECK-NEXT: [[A:%.*]] = icmp eq i64 [[X:%.*]], 0 ; CHECK-NEXT: br i1 [[A]], label [[EXIT:%.*]], label [[NON_ZERO:%.*]] ; CHECK: non_zero: -; CHECK-NEXT: br i1 true, label [[EXIT]], label [[UNREACHABLE:%.*]] +; CHECK-NEXT: [[B:%.*]] = icmp ugt i64 [[X]], 0 +; CHECK-NEXT: br i1 [[B]], label [[EXIT]], label [[UNREACHABLE:%.*]] ; CHECK: unreachable: ; CHECK-NEXT: br label [[EXIT]] ; CHECK: exit: @@ -71,9 +73,11 @@ define i1 @test2(i64 %x, i1 %y) { ; CHECK: two: ; CHECK-NEXT: br label [[MAINBLOCK]] ; CHECK: mainblock: +; CHECK-NEXT: [[P:%.*]] = phi i64 [ [[X]], [[ONE]] ], [ 42, [[TWO]] ] +; CHECK-NEXT: [[CMP:%.*]] = icmp eq i64 [[P]], 0 ; CHECK-NEXT: br label [[EXIT]] ; CHECK: exit: -; CHECK-NEXT: [[RES:%.*]] = phi i1 [ false, [[MAINBLOCK]] ], [ true, [[START:%.*]] ] +; CHECK-NEXT: [[RES:%.*]] = phi i1 [ [[CMP]], [[MAINBLOCK]] ], [ true, [[START:%.*]] ] ; CHECK-NEXT: ret i1 [[RES]] ; start: diff --git a/llvm/test/Transforms/LICM/hoist-mustexec.ll b/llvm/test/Transforms/LICM/hoist-mustexec.ll index 59184eb542cf..521d35296572 100644 --- a/llvm/test/Transforms/LICM/hoist-mustexec.ll +++ b/llvm/test/Transforms/LICM/hoist-mustexec.ll @@ -129,6 +129,8 @@ fail: } ; requires fact length is non-zero +; TODO: IsKnownNonNullFromDominatingConditions is currently only be done for +; pointers; should handle integers too define i32 @test4(i32* noalias nocapture readonly %a) nounwind uwtable { ; CHECK-LABEL: @test4( ; CHECK-NEXT: entry: @@ -136,7 +138,6 @@ define i32 @test4(i32* noalias nocapture readonly %a) nounwind uwtable { ; CHECK-NEXT: [[IS_ZERO:%.*]] = icmp eq i32 [[LEN]], 0 ; CHECK-NEXT: br i1 [[IS_ZERO]], label [[FAIL:%.*]], label [[PREHEADER:%.*]] ; CHECK: preheader: -; CHECK-NEXT: [[I1:%.*]] = load i32, i32* [[A]], align 4 ; CHECK-NEXT: br label [[FOR_BODY:%.*]] ; CHECK: for.body: ; CHECK-NEXT: [[IV:%.*]] = phi i32 [ 0, [[PREHEADER]] ], [ [[INC:%.*]], [[CONTINUE:%.*]] ] @@ -144,6 +145,7 @@ define i32 @test4(i32* noalias nocapture readonly %a) nounwind uwtable { ; CHECK-NEXT: [[R_CHK:%.*]] = icmp ult i32 [[IV]], [[LEN]] ; CHECK-NEXT: br i1 [[R_CHK]], label [[CONTINUE]], label [[FAIL_LOOPEXIT:%.*]] ; CHECK: continue: +; CHECK-NEXT: [[I1:%.*]] = load i32, i32* [[A]], align 4 ; CHECK-NEXT: [[ADD]] = add nsw i32 [[I1]], [[ACC]] ; CHECK-NEXT: [[INC]] = add nuw nsw i32 [[IV]], 1 ; CHECK-NEXT: [[EXITCOND:%.*]] = icmp eq i32 [[INC]], 1000 -- GitLab