From 9a31ece3941ddfd99e7d6bc4377aa671a278e04f Mon Sep 17 00:00:00 2001 From: Sanjay Patel Date: Sat, 12 Aug 2017 16:41:08 +0000 Subject: [PATCH] [BDCE] clear poison generators after turning a value into zero (PR33695, PR34037) nsw, nuw, and exact carry implicit assumptions about their operands, so we need to clear those after trivializing a value. We decided there was no danger for llvm.assume or metadata, so there's just a comment about that. This fixes miscompiles as shown in: https://bugs.llvm.org/show_bug.cgi?id=33695 https://bugs.llvm.org/show_bug.cgi?id=34037 Differential Revision: https://reviews.llvm.org/D36592 llvm-svn: 310779 --- lib/Transforms/Scalar/BDCE.cpp | 47 +++++++++++++++++++ .../Transforms/BDCE/invalidate-assumptions.ll | 39 +++++++++++++-- 2 files changed, 81 insertions(+), 5 deletions(-) diff --git a/lib/Transforms/Scalar/BDCE.cpp b/lib/Transforms/Scalar/BDCE.cpp index 61e8700f1cd..5677ac91d6f 100644 --- a/lib/Transforms/Scalar/BDCE.cpp +++ b/lib/Transforms/Scalar/BDCE.cpp @@ -15,6 +15,7 @@ //===----------------------------------------------------------------------===// #include "llvm/Transforms/Scalar/BDCE.h" +#include "llvm/ADT/SmallPtrSet.h" #include "llvm/ADT/SmallVector.h" #include "llvm/ADT/Statistic.h" #include "llvm/Analysis/DemandedBits.h" @@ -35,6 +36,49 @@ using namespace llvm; STATISTIC(NumRemoved, "Number of instructions removed (unused)"); STATISTIC(NumSimplified, "Number of instructions trivialized (dead bits)"); +/// If an instruction is trivialized (dead), then the chain of users of that +/// instruction may need to be cleared of assumptions that can no longer be +/// guaranteed correct. +static void clearAssumptionsOfUsers(Instruction *I, DemandedBits &DB) { + // Any value we're trivializing should be an integer value, and moreover, + // any conversion between an integer value and a non-integer value should + // demand all of the bits. This will cause us to stop looking down the + // use/def chain, so we should only see integer-typed instructions here. + auto isExternallyVisible = [](Instruction *I, DemandedBits &DB) { + assert(I->getType()->isIntegerTy() && "Trivializing a non-integer value?"); + return DB.getDemandedBits(I).isAllOnesValue(); + }; + + // Initialize the worklist with eligible direct users. + SmallVector WorkList; + for (User *JU : I->users()) { + auto *J = dyn_cast(JU); + if (J && !isExternallyVisible(J, DB)) + WorkList.push_back(J); + } + + // DFS through subsequent users while tracking visits to avoid cycles. + SmallPtrSet Visited; + while (!WorkList.empty()) { + Instruction *J = WorkList.pop_back_val(); + + // NSW, NUW, and exact are based on operands that might have changed. + J->dropPoisonGeneratingFlags(); + + // We do not have to worry about llvm.assume or range metadata: + // 1. llvm.assume demands its operand, so trivializing can't change it. + // 2. range metadata only applies to memory accesses which demand all bits. + + Visited.insert(J); + + for (User *KU : J->users()) { + auto *K = dyn_cast(KU); + if (K && !Visited.count(K) && !isExternallyVisible(K, DB)) + WorkList.push_back(K); + } + } +} + static bool bitTrackingDCE(Function &F, DemandedBits &DB) { SmallVector Worklist; bool Changed = false; @@ -51,6 +95,9 @@ static bool bitTrackingDCE(Function &F, DemandedBits &DB) { // replacing all uses with something else. Then, if they don't need to // remain live (because they have side effects, etc.) we can remove them. DEBUG(dbgs() << "BDCE: Trivializing: " << I << " (all bits dead)\n"); + + clearAssumptionsOfUsers(&I, DB); + // FIXME: In theory we could substitute undef here instead of zero. // This should be reconsidered once we settle on the semantics of // undef, poison, etc. diff --git a/test/Transforms/BDCE/invalidate-assumptions.ll b/test/Transforms/BDCE/invalidate-assumptions.ll index ac337c44ada..094c5ec6bd9 100644 --- a/test/Transforms/BDCE/invalidate-assumptions.ll +++ b/test/Transforms/BDCE/invalidate-assumptions.ll @@ -1,6 +1,6 @@ ; RUN: opt -bdce %s -S | FileCheck %s -; FIXME: The 'nuw' on the subtract allows us to deduce that %setbit is not demanded. +; The 'nuw' on the subtract allows us to deduce that %setbit is not demanded. ; But if we change that value to '0', then the 'nuw' is no longer valid. If we don't ; remove the 'nuw', another pass (-instcombine) may make a transform based on an ; that incorrect assumption and we can miscompile: @@ -11,7 +11,7 @@ define i1 @PR33695(i1 %b, i8 %x) { ; CHECK-NEXT: [[SETBIT:%.*]] = or i8 %x, 64 ; CHECK-NEXT: [[LITTLE_NUMBER:%.*]] = zext i1 %b to i8 ; CHECK-NEXT: [[BIG_NUMBER:%.*]] = shl i8 0, 1 -; CHECK-NEXT: [[SUB:%.*]] = sub nuw i8 [[BIG_NUMBER]], [[LITTLE_NUMBER]] +; CHECK-NEXT: [[SUB:%.*]] = sub i8 [[BIG_NUMBER]], [[LITTLE_NUMBER]] ; CHECK-NEXT: [[TRUNC:%.*]] = trunc i8 [[SUB]] to i1 ; CHECK-NEXT: ret i1 [[TRUNC]] ; @@ -23,7 +23,7 @@ define i1 @PR33695(i1 %b, i8 %x) { ret i1 %trunc } -; FIXME: Similar to above, but now with more no-wrap. +; Similar to above, but now with more no-wrap. ; https://bugs.llvm.org/show_bug.cgi?id=34037 define i64 @PR34037(i64 %m, i32 %r, i64 %j, i1 %b, i32 %k, i64 %p) { @@ -34,9 +34,9 @@ define i64 @PR34037(i64 %m, i32 %r, i64 %j, i1 %b, i32 %k, i64 %p) { ; CHECK-NEXT: [[OR:%.*]] = or i64 %j, 0 ; CHECK-NEXT: [[SHL:%.*]] = shl i64 0, 29 ; CHECK-NEXT: [[CONV1:%.*]] = select i1 %b, i64 7, i64 0 -; CHECK-NEXT: [[SUB:%.*]] = sub nuw nsw i64 [[SHL]], [[CONV1]] +; CHECK-NEXT: [[SUB:%.*]] = sub i64 [[SHL]], [[CONV1]] ; CHECK-NEXT: [[CONV2:%.*]] = zext i32 %k to i64 -; CHECK-NEXT: [[MUL:%.*]] = mul nsw i64 [[SUB]], [[CONV2]] +; CHECK-NEXT: [[MUL:%.*]] = mul i64 [[SUB]], [[CONV2]] ; CHECK-NEXT: [[CONV4:%.*]] = and i64 %p, 65535 ; CHECK-NEXT: [[AND5:%.*]] = and i64 [[MUL]], [[CONV4]] ; CHECK-NEXT: ret i64 [[AND5]] @@ -55,3 +55,32 @@ define i64 @PR34037(i64 %m, i32 %r, i64 %j, i1 %b, i32 %k, i64 %p) { ret i64 %and5 } +; This is a manufactured example based on the 1st test to prove that the +; assumption-killing algorithm stops at the call. Ie, it does not remove +; nsw/nuw from the 'add' because a call demands all bits of its argument. + +declare i1 @foo(i1) + +define i1 @poison_on_call_user_is_ok(i1 %b, i8 %x) { +; CHECK-LABEL: @poison_on_call_user_is_ok( +; CHECK-NEXT: [[SETBIT:%.*]] = or i8 %x, 64 +; CHECK-NEXT: [[LITTLE_NUMBER:%.*]] = zext i1 %b to i8 +; CHECK-NEXT: [[BIG_NUMBER:%.*]] = shl i8 0, 1 +; CHECK-NEXT: [[SUB:%.*]] = sub i8 [[BIG_NUMBER]], [[LITTLE_NUMBER]] +; CHECK-NEXT: [[TRUNC:%.*]] = trunc i8 [[SUB]] to i1 +; CHECK-NEXT: [[CALL_RESULT:%.*]] = call i1 @foo(i1 [[TRUNC]]) +; CHECK-NEXT: [[ADD:%.*]] = add nuw nsw i1 [[CALL_RESULT]], true +; CHECK-NEXT: [[MUL:%.*]] = mul i1 [[TRUNC]], [[ADD]] +; CHECK-NEXT: ret i1 [[MUL]] +; + %setbit = or i8 %x, 64 + %little_number = zext i1 %b to i8 + %big_number = shl i8 %setbit, 1 + %sub = sub nuw i8 %big_number, %little_number + %trunc = trunc i8 %sub to i1 + %call_result = call i1 @foo(i1 %trunc) + %add = add nsw nuw i1 %call_result, 1 + %mul = mul i1 %trunc, %add + ret i1 %mul +} +