[llvm-branch-commits] [llvm-branch] r310898 - Merging r309945, r310779 and r310842:
Hans Wennborg via llvm-branch-commits
llvm-branch-commits at lists.llvm.org
Mon Aug 14 17:06:24 PDT 2017
Author: hans
Date: Mon Aug 14 17:06:23 2017
New Revision: 310898
URL: http://llvm.org/viewvc/llvm-project?rev=310898&view=rev
Log:
Merging r309945, r310779 and r310842:
------------------------------------------------------------------------
r309945 | spatel | 2017-08-03 08:07:37 -0700 (Thu, 03 Aug 2017) | 2 lines
[BDCE] add tests to show invalid/incomplete transforms
------------------------------------------------------------------------
------------------------------------------------------------------------
r310779 | spatel | 2017-08-12 09:41:08 -0700 (Sat, 12 Aug 2017) | 13 lines
[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
------------------------------------------------------------------------
------------------------------------------------------------------------
r310842 | spatel | 2017-08-14 08:13:46 -0700 (Mon, 14 Aug 2017) | 16 lines
[BDCE] reduce scope of an assert (PR34179)
The assert was added with r310779 and is usually correct,
but as the test shows, not always. The 'volatile' on the
load is needed to expose the faulty path because without
it, DemandedBits would return that the load is just dead
rather than not demanded, and so we wouldn't hit the
bogus assert.
Also, since the lambda is just a single-line now, get rid
of it and inline the DB.isAllOnesValue() calls.
This should fix (prevent execution of a faulty assert):
https://bugs.llvm.org/show_bug.cgi?id=34179
------------------------------------------------------------------------
Added:
llvm/branches/release_50/test/Transforms/BDCE/invalidate-assumptions.ll
- copied, changed from r309945, llvm/trunk/test/Transforms/BDCE/invalidate-assumptions.ll
Modified:
llvm/branches/release_50/ (props changed)
llvm/branches/release_50/lib/Transforms/Scalar/BDCE.cpp
Propchange: llvm/branches/release_50/
------------------------------------------------------------------------------
--- svn:mergeinfo (original)
+++ svn:mergeinfo Mon Aug 14 17:06:23 2017
@@ -1,3 +1,3 @@
/llvm/branches/Apple/Pertwee:110850,110961
/llvm/branches/type-system-rewrite:133420-134817
-/llvm/trunk:155241,308483-308484,308503,308808,308813,308847,308891,308906,308950,308963,308978,308986,309044,309071,309113,309120,309122,309140,309227,309302,309321,309323,309325,309330,309343,309353,309355,309422,309481,309483,309495,309555,309561,309594,309614,309651,309744,309758,309849,309928,309930,310071,310190,310240-310242,310250,310253,310267,310481,310492,310510,310534,310552,310604
+/llvm/trunk:155241,308483-308484,308503,308808,308813,308847,308891,308906,308950,308963,308978,308986,309044,309071,309113,309120,309122,309140,309227,309302,309321,309323,309325,309330,309343,309353,309355,309422,309481,309483,309495,309555,309561,309594,309614,309651,309744,309758,309849,309928,309930,309945,310071,310190,310240-310242,310250,310253,310267,310481,310492,310510,310534,310552,310604,310779,310842
Modified: llvm/branches/release_50/lib/Transforms/Scalar/BDCE.cpp
URL: http://llvm.org/viewvc/llvm-project/llvm/branches/release_50/lib/Transforms/Scalar/BDCE.cpp?rev=310898&r1=310897&r2=310898&view=diff
==============================================================================
--- llvm/branches/release_50/lib/Transforms/Scalar/BDCE.cpp (original)
+++ llvm/branches/release_50/lib/Transforms/Scalar/BDCE.cpp Mon Aug 14 17:06:23 2017
@@ -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,46 @@ 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) {
+ assert(I->getType()->isIntegerTy() && "Trivializing a non-integer value?");
+
+ // Initialize the worklist with eligible direct users.
+ SmallVector<Instruction *, 16> WorkList;
+ for (User *JU : I->users()) {
+ // If all bits of a user are demanded, then we know that nothing below that
+ // in the def-use chain needs to be changed.
+ auto *J = dyn_cast<Instruction>(JU);
+ if (J && !DB.getDemandedBits(J).isAllOnesValue())
+ WorkList.push_back(J);
+ }
+
+ // DFS through subsequent users while tracking visits to avoid cycles.
+ SmallPtrSet<Instruction *, 16> 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()) {
+ // If all bits of a user are demanded, then we know that nothing below
+ // that in the def-use chain needs to be changed.
+ auto *K = dyn_cast<Instruction>(KU);
+ if (K && !Visited.count(K) && !DB.getDemandedBits(K).isAllOnesValue())
+ WorkList.push_back(K);
+ }
+ }
+}
+
static bool bitTrackingDCE(Function &F, DemandedBits &DB) {
SmallVector<Instruction*, 128> Worklist;
bool Changed = false;
@@ -51,6 +92,9 @@ static bool bitTrackingDCE(Function &F,
// 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.
Copied: llvm/branches/release_50/test/Transforms/BDCE/invalidate-assumptions.ll (from r309945, llvm/trunk/test/Transforms/BDCE/invalidate-assumptions.ll)
URL: http://llvm.org/viewvc/llvm-project/llvm/branches/release_50/test/Transforms/BDCE/invalidate-assumptions.ll?p2=llvm/branches/release_50/test/Transforms/BDCE/invalidate-assumptions.ll&p1=llvm/trunk/test/Transforms/BDCE/invalidate-assumptions.ll&r1=309945&r2=310898&rev=310898&view=diff
==============================================================================
--- llvm/trunk/test/Transforms/BDCE/invalidate-assumptions.ll (original)
+++ llvm/branches/release_50/test/Transforms/BDCE/invalidate-assumptions.ll Mon Aug 14 17:06:23 2017
@@ -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
; 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,46 @@ define i64 @PR34037(i64 %m, i32 %r, i64
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
+}
+
+
+; We were asserting that all users of a trivialized integer-type instruction were
+; also integer-typed, but that's too strong. The alloca has a pointer-type result.
+
+define void @PR34179(i32* %a) {
+; CHECK-LABEL: @PR34179(
+; CHECK-NEXT: [[T0:%.*]] = load volatile i32, i32* %a
+; CHECK-NEXT: ret void
+;
+ %t0 = load volatile i32, i32* %a
+ %vla = alloca i32, i32 %t0
+ ret void
+}
+
More information about the llvm-branch-commits
mailing list