Skip to content

Commit

Permalink
[IRCE] Fix intersection between signed and unsigned ranges
Browse files Browse the repository at this point in the history 
IRCE for unsigned latch conditions was temporarily disabled by rL314881. The motivating
example contained an unsigned latch condition and a signed range check. One of the safe
iteration ranges was `[1, SINT_MAX + 1]`. Its right border was incorrectly interpreted as a negative
value in `IntersectRange` function, this lead to a miscompile under which we deleted a range check
without inserting a postloop where it was needed.

This patch brings back IRCE for unsigned latch conditions. Now we treat range intersection more
carefully. If the latch condition was unsigned, we only try to consider a range check for deletion if:
1. The range check is also unsigned, or
2. Safe iteration range of the range check lies within `[0, SINT_MAX]`.
The same is done for signed latch.

Values from `[0, SINT_MAX]` are unambiguous, these values are non-negative under any interpretation,
and all values of a range intersected with such range are also non-negative.

We also use signed/unsigned min/max functions for range intersection depending on type of the
latch condition.

Differential Revision: https://reviews.llvm.org/D38581

llvm-svn: 316552
  • Loading branch information
Max Kazantsev committed Oct 25, 2017
1 parent 279790b commit 9ac7021
Show file tree
Hide file tree
Showing 9 changed files with 402 additions and 37 deletions.
87 changes: 73 additions & 14 deletions llvm/lib/Transforms/Scalar/InductiveRangeCheckElimination.cpp
View file
Expand Up @@ -112,7 +112,7 @@ static cl::opt<bool> SkipProfitabilityChecks("irce-skip-profitability-checks",
cl::Hidden, cl::init(false));

static cl::opt<bool> AllowUnsignedLatchCondition("irce-allow-unsigned-latch",
cl::Hidden, cl::init(false));
cl::Hidden, cl::init(true));

static const char *ClonedLoopTag = "irce.loop.clone";

Expand Down Expand Up @@ -154,10 +154,11 @@ class InductiveRangeCheck {
Value *Length = nullptr;
Use *CheckUse = nullptr;
RangeCheckKind Kind = RANGE_CHECK_UNKNOWN;
bool IsSigned = true;

static RangeCheckKind parseRangeCheckICmp(Loop *L, ICmpInst *ICI,
ScalarEvolution &SE, Value *&Index,
Value *&Length);
Value *&Length, bool &IsSigned);

static void
extractRangeChecksFromCond(Loop *L, ScalarEvolution &SE, Use &ConditionUse,
Expand All @@ -168,6 +169,7 @@ class InductiveRangeCheck {
const SCEV *getOffset() const { return Offset; }
const SCEV *getScale() const { return Scale; }
Value *getLength() const { return Length; }
bool isSigned() const { return IsSigned; }

void print(raw_ostream &OS) const {
OS << "InductiveRangeCheck:\n";
Expand Down Expand Up @@ -295,7 +297,7 @@ StringRef InductiveRangeCheck::rangeCheckKindToStr(
InductiveRangeCheck::RangeCheckKind
InductiveRangeCheck::parseRangeCheckICmp(Loop *L, ICmpInst *ICI,
ScalarEvolution &SE, Value *&Index,
Value *&Length) {
Value *&Length, bool &IsSigned) {
auto IsNonNegativeAndNotLoopVarying = [&SE, L](Value *V) {
const SCEV *S = SE.getSCEV(V);
if (isa<SCEVCouldNotCompute>(S))
Expand All @@ -317,6 +319,7 @@ InductiveRangeCheck::parseRangeCheckICmp(Loop *L, ICmpInst *ICI,
std::swap(LHS, RHS);
LLVM_FALLTHROUGH;
case ICmpInst::ICMP_SGE:
IsSigned = true;
if (match(RHS, m_ConstantInt<0>())) {
Index = LHS;
return RANGE_CHECK_LOWER;
Expand All @@ -327,6 +330,7 @@ InductiveRangeCheck::parseRangeCheckICmp(Loop *L, ICmpInst *ICI,
std::swap(LHS, RHS);
LLVM_FALLTHROUGH;
case ICmpInst::ICMP_SGT:
IsSigned = true;
if (match(RHS, m_ConstantInt<-1>())) {
Index = LHS;
return RANGE_CHECK_LOWER;
Expand All @@ -343,6 +347,7 @@ InductiveRangeCheck::parseRangeCheckICmp(Loop *L, ICmpInst *ICI,
std::swap(LHS, RHS);
LLVM_FALLTHROUGH;
case ICmpInst::ICMP_UGT:
IsSigned = false;
if (IsNonNegativeAndNotLoopVarying(LHS)) {
Index = RHS;
Length = LHS;
Expand Down Expand Up @@ -375,7 +380,8 @@ void InductiveRangeCheck::extractRangeChecksFromCond(
const auto &RChkA = SubChecks[0];
const auto &RChkB = SubChecks[1];
if ((RChkA.Length == RChkB.Length || !RChkA.Length || !RChkB.Length) &&
RChkA.Offset == RChkB.Offset && RChkA.Scale == RChkB.Scale) {
RChkA.Offset == RChkB.Offset && RChkA.Scale == RChkB.Scale &&
RChkA.IsSigned == RChkB.IsSigned) {
// If RChkA.Kind == RChkB.Kind then we just found two identical checks.
// But if one of them is a RANGE_CHECK_LOWER and the other is a
// RANGE_CHECK_UPPER (only possibility if they're different) then
Expand All @@ -384,6 +390,7 @@ void InductiveRangeCheck::extractRangeChecksFromCond(
(InductiveRangeCheck::RangeCheckKind)(RChkA.Kind | RChkB.Kind);
SubChecks[0].Length = RChkA.Length ? RChkA.Length : RChkB.Length;
SubChecks[0].CheckUse = &ConditionUse;
SubChecks[0].IsSigned = RChkA.IsSigned;

// We updated one of the checks in place, now erase the other.
SubChecks.pop_back();
Expand All @@ -399,7 +406,8 @@ void InductiveRangeCheck::extractRangeChecksFromCond(
return;

Value *Length = nullptr, *Index;
auto RCKind = parseRangeCheckICmp(L, ICI, SE, Index, Length);
bool IsSigned;
auto RCKind = parseRangeCheckICmp(L, ICI, SE, Index, Length, IsSigned);
if (RCKind == InductiveRangeCheck::RANGE_CHECK_UNKNOWN)
return;

Expand All @@ -416,6 +424,7 @@ void InductiveRangeCheck::extractRangeChecksFromCond(
IRC.Scale = IndexAddRec->getStepRecurrence(SE);
IRC.CheckUse = &ConditionUse;
IRC.Kind = RCKind;
IRC.IsSigned = IsSigned;
Checks.push_back(IRC);
}

Expand Down Expand Up @@ -917,9 +926,6 @@ LoopStructure::parseLoopStructure(ScalarEvolution &SE,
IsSignedPredicate =
Pred == ICmpInst::ICMP_SLT || Pred == ICmpInst::ICMP_SGT;

// FIXME: We temporarily disable unsigned latch conditions by default
// because of found problems with intersecting signed and unsigned ranges.
// We are going to turn it on once the problems are fixed.
if (!IsSignedPredicate && !AllowUnsignedLatchCondition) {
FailureReason = "unsigned latch conditions are explicitly prohibited";
return None;
Expand Down Expand Up @@ -1001,9 +1007,6 @@ LoopStructure::parseLoopStructure(ScalarEvolution &SE,
IsSignedPredicate =
Pred == ICmpInst::ICMP_SLT || Pred == ICmpInst::ICMP_SGT;

// FIXME: We temporarily disable unsigned latch conditions by default
// because of found problems with intersecting signed and unsigned ranges.
// We are going to turn it on once the problems are fixed.
if (!IsSignedPredicate && !AllowUnsignedLatchCondition) {
FailureReason = "unsigned latch conditions are explicitly prohibited";
return None;
Expand Down Expand Up @@ -1670,9 +1673,9 @@ InductiveRangeCheck::computeSafeIterationSpace(
}

static Optional<InductiveRangeCheck::Range>
IntersectRange(ScalarEvolution &SE,
const Optional<InductiveRangeCheck::Range> &R1,
const InductiveRangeCheck::Range &R2) {
IntersectSignedRange(ScalarEvolution &SE,
const Optional<InductiveRangeCheck::Range> &R1,
const InductiveRangeCheck::Range &R2) {
if (R2.isEmpty(SE, /* IsSigned */ true))
return None;
if (!R1.hasValue())
Expand All @@ -1698,6 +1701,35 @@ IntersectRange(ScalarEvolution &SE,
return Ret;
}

static Optional<InductiveRangeCheck::Range>
IntersectUnsignedRange(ScalarEvolution &SE,
const Optional<InductiveRangeCheck::Range> &R1,
const InductiveRangeCheck::Range &R2) {
if (R2.isEmpty(SE, /* IsSigned */ false))
return None;
if (!R1.hasValue())
return R2;
auto &R1Value = R1.getValue();
// We never return empty ranges from this function, and R1 is supposed to be
// a result of intersection. Thus, R1 is never empty.
assert(!R1Value.isEmpty(SE, /* IsSigned */ false) &&
"We should never have empty R1!");

// TODO: we could widen the smaller range and have this work; but for now we
// bail out to keep things simple.
if (R1Value.getType() != R2.getType())
return None;

const SCEV *NewBegin = SE.getUMaxExpr(R1Value.getBegin(), R2.getBegin());
const SCEV *NewEnd = SE.getUMinExpr(R1Value.getEnd(), R2.getEnd());

// If the resulting range is empty, just return None.
auto Ret = InductiveRangeCheck::Range(NewBegin, NewEnd);
if (Ret.isEmpty(SE, /* IsSigned */ false))
return None;
return Ret;
}

bool InductiveRangeCheckElimination::runOnLoop(Loop *L, LPPassManager &LPM) {
if (skipLoop(L))
return false;
Expand Down Expand Up @@ -1756,11 +1788,38 @@ bool InductiveRangeCheckElimination::runOnLoop(Loop *L, LPPassManager &LPM) {
Instruction *ExprInsertPt = Preheader->getTerminator();

SmallVector<InductiveRangeCheck, 4> RangeChecksToEliminate;
auto RangeIsNonNegative = [&](InductiveRangeCheck::Range &R) {
return SE.isKnownNonNegative(R.getBegin()) &&
SE.isKnownNonNegative(R.getEnd());
};
// Basing on the type of latch predicate, we interpret the IV iteration range
// as signed or unsigned range. We use different min/max functions (signed or
// unsigned) when intersecting this range with safe iteration ranges implied
// by range checks.
auto IntersectRange =
LS.IsSignedPredicate ? IntersectSignedRange : IntersectUnsignedRange;

IRBuilder<> B(ExprInsertPt);
for (InductiveRangeCheck &IRC : RangeChecks) {
auto Result = IRC.computeSafeIterationSpace(SE, IndVar);
if (Result.hasValue()) {
// Intersecting a signed and an unsigned ranges may produce incorrect
// results because we can use neither signed nor unsigned min/max for
// reliably correct intersection if a range contains negative values
// which are either actually negative or big positive. Intersection is
// safe in two following cases:
// 1. Both ranges are signed/unsigned, then we use signed/unsigned min/max
// respectively for their intersection;
// 2. IRC safe iteration space only contains values from [0, SINT_MAX].
// The interpretation of these values is unambiguous.
// We take the type of IV iteration range as a reference (we will
// intersect it with the resulting range of all IRC's later in
// calculateSubRanges). Only ranges of IRC of the same type are considered
// for removal unless we prove that its range doesn't contain ambiguous
// values.
if (IRC.isSigned() != LS.IsSignedPredicate &&
!RangeIsNonNegative(Result.getValue()))
continue;
auto MaybeSafeIterRange =
IntersectRange(SE, SafeIterRange, Result.getValue());
if (MaybeSafeIterRange.hasValue()) {
Expand Down
13 changes: 6 additions & 7 deletions llvm/test/Transforms/IRCE/clamp.ll
View file
@@ -1,8 +1,4 @@
; This test demonstrates the confusion in ranges: we have unsigned ranges here,
; but signed comparisons in IntersectRanges produce bad results. We temporarily
; disable it and re-enable once the unsigned ranges are supported again.
; XFAIL: *
; RUN: opt -verify-loop-info -irce-print-changed-loops -irce -irce-allow-unsigned-latch=true -S < %s 2>&1 | FileCheck %s
; RUN: opt -verify-loop-info -irce-print-changed-loops -irce -S < %s 2>&1 | FileCheck %s

; The test demonstrates that incorrect behavior of Clamp may lead to incorrect
; calculation of post-loop exit condition.
Expand All @@ -27,7 +23,10 @@ preheader: ; preds = %entry
; CHECK-NEXT: %length_gep.i146 = getelementptr inbounds i8, i8 addrspace(1)* undef, i64 8
; CHECK-NEXT: %length_gep_typed.i147 = bitcast i8 addrspace(1)* undef to i32 addrspace(1)*
; CHECK-NEXT: %tmp43 = icmp ult i64 %indvars.iv.next467, %tmp21
; CHECK-NEXT: br i1 false, label %loop.preheader, label %main.pseudo.exit
; CHECK-NEXT: [[C0:%[^ ]+]] = icmp ugt i64 %tmp21, 1
; CHECK-NEXT: %exit.mainloop.at = select i1 [[C0]], i64 %tmp21, i64 1
; CHECK-NEXT: [[C1:%[^ ]+]] = icmp ult i64 1, %exit.mainloop.at
; CHECK-NEXT: br i1 [[C1]], label %loop.preheader, label %main.pseudo.exit

%length_gep.i146 = getelementptr inbounds i8, i8 addrspace(1)* undef, i64 8
%length_gep_typed.i147 = bitcast i8 addrspace(1)* undef to i32 addrspace(1)*
Expand All @@ -37,7 +36,7 @@ preheader: ; preds = %entry
not_zero: ; preds = %in_bounds
; CHECK: not_zero:
; CHECK: %tmp56 = icmp ult i64 %indvars.iv.next, %tmp21
; CHECK-NEXT: [[COND:%[^ ]+]] = icmp ult i64 %indvars.iv.next, 1
; CHECK-NEXT: [[COND:%[^ ]+]] = icmp ult i64 %indvars.iv.next, %exit.mainloop.at
; CHECK-NEXT: br i1 [[COND]], label %loop, label %main.exit.selector

%tmp51 = trunc i64 %indvars.iv.next to i32
Expand Down
8 changes: 4 additions & 4 deletions llvm/test/Transforms/IRCE/correct-loop-info.ll
View file
Expand Up @@ -31,7 +31,7 @@ define void @baz() personality i32* ()* @ham {
; CHECK-NEXT: to label [[BB5:%.*]] unwind label %outer_exiting.loopexit.split-lp.loopexit.split-lp
; CHECK: bb5:
; CHECK-NEXT: [[TMP6]] = add i32 [[TMP4]], 1
; CHECK-NEXT: [[TMP7:%.*]] = icmp ult i32 [[TMP6]], 1
; CHECK-NEXT: [[TMP7:%.*]] = icmp slt i32 [[TMP6]], 1
; CHECK-NEXT: br i1 true, label [[BB8]], label [[EXIT3_LOOPEXIT5:%.*]]
; CHECK: bb8:
; CHECK-NEXT: [[TMP9:%.*]] = icmp slt i32 [[TMP6]], 84
Expand Down Expand Up @@ -90,7 +90,7 @@ define void @baz() personality i32* ()* @ham {
; CHECK-NEXT: to label [[BB5_PRELOOP:%.*]] unwind label [[OUTER_EXITING_LOOPEXIT:%.*]]
; CHECK: bb5.preloop:
; CHECK-NEXT: [[TMP6_PRELOOP]] = add i32 [[TMP4_PRELOOP]], 1
; CHECK-NEXT: [[TMP7_PRELOOP:%.*]] = icmp ult i32 [[TMP6_PRELOOP]], 1
; CHECK-NEXT: [[TMP7_PRELOOP:%.*]] = icmp slt i32 [[TMP6_PRELOOP]], 1
; CHECK-NEXT: br i1 [[TMP7_PRELOOP]], label [[BB8_PRELOOP]], label [[EXIT3_LOOPEXIT:%.*]]
; CHECK: bb8.preloop:
; CHECK-NEXT: [[TMP9_PRELOOP:%.*]] = icmp slt i32 [[TMP6_PRELOOP]], 84
Expand All @@ -112,7 +112,7 @@ define void @baz() personality i32* ()* @ham {
; CHECK-NEXT: to label [[BB5_POSTLOOP:%.*]] unwind label %outer_exiting.loopexit.split-lp.loopexit
; CHECK: bb5.postloop:
; CHECK-NEXT: [[TMP6_POSTLOOP]] = add i32 [[TMP4_POSTLOOP]], 1
; CHECK-NEXT: [[TMP7_POSTLOOP:%.*]] = icmp ult i32 [[TMP6_POSTLOOP]], 1
; CHECK-NEXT: [[TMP7_POSTLOOP:%.*]] = icmp slt i32 [[TMP6_POSTLOOP]], 1
; CHECK-NEXT: br i1 [[TMP7_POSTLOOP]], label [[BB8_POSTLOOP]], label [[EXIT3_LOOPEXIT4:%.*]]
; CHECK: bb8.postloop:
; CHECK-NEXT: [[TMP9_POSTLOOP:%.*]] = icmp slt i32 [[TMP6_POSTLOOP]], 84
Expand All @@ -135,7 +135,7 @@ innerheader: ; preds = %bb8, %bb2

bb5: ; preds = %innerheader
%tmp6 = add i32 %tmp4, 1
%tmp7 = icmp ult i32 %tmp6, 1
%tmp7 = icmp slt i32 %tmp6, 1
br i1 %tmp7, label %bb8, label %exit3

bb8: ; preds = %bb5
Expand Down
3 changes: 1 addition & 2 deletions llvm/test/Transforms/IRCE/empty_ranges.ll
View file
@@ -1,5 +1,4 @@
; RUN: opt -verify-loop-info -irce-print-changed-loops -irce -irce-allow-unsigned-latch=true -S

; RUN: opt -verify-loop-info -irce-print-changed-loops -irce -S

; Make sure that IRCE doesn't apply in case of empty ranges.
; (i + 30 < 40) if i in [-30, 10).
Expand Down
2 changes: 1 addition & 1 deletion llvm/test/Transforms/IRCE/eq_ne.ll
View file
@@ -1,4 +1,4 @@
; RUN: opt -verify-loop-info -irce-print-changed-loops -irce -irce-allow-unsigned-latch=true -S < %s 2>&1 | FileCheck %s
; RUN: opt -verify-loop-info -irce-print-changed-loops -irce -S < %s 2>&1 | FileCheck %s

; CHECK: irce: in function test_01: constrained Loop at depth 1 containing: %loop<header><exiting>,%in.bounds<latch><exiting>
; CHECK: irce: in function test_01u: constrained Loop at depth 1 containing: %loop<header><exiting>,%in.bounds<latch><exiting>
Expand Down

0 comments on commit 9ac7021

Please sign in to comment.