This is an archive of the discontinued LLVM Phabricator instance.

[ValueTracking] Honour recursion limit
ClosedPublic

Authored by davide on Aug 9 2017, 5:05 AM.

Details

Summary

The recently improved support for icmp in ValueTracking,

commit c6b02fa9154c7b897afe5fb746cec527fb3f8bb0
Author: Chad Rosier <mcrosier@codeaurora.org>
Date:   Thu Jul 6 20:00:25 2017 +0000

    [ValueTracking] Support icmps fed by 'and' and 'or'.

    This patch adds support for handling some forms of ands and ors in
    ValueTracking's isImpliedCondition API.

    PR33611
    https://reviews.llvm.org/D34901

exposes the fact that isImplied condition doesn't really bail out if we hit the recursion limit (and this function calls computeKnownBits which increases the depth and asserts).

This is the top of the stack, for archeologists (the frame is #13 because this is running inside rr).

#13 0x000000000215d12f in llvm::computeKnownBits (V=0x7a05fc0, Known=..., DL=...,
    Depth=7, AC=0x0, CxtI=0x0, DT=0x0, ORE=0x0) at ../lib/Analysis/ValueTracking.cpp:142
#14 0x0000000002171143 in isTruePredicate(llvm::CmpInst::Predicate, llvm::Value const*, llvm::Value const*, llvm::DataLayout const&, unsigned int)::$_9::operator()(llvm::Value const
*, llvm::Value const*, llvm::Value const*&, llvm::APInt const*&, llvm::APInt const*&) const (this=0x7ffd945de098, A=0x7a078d0, B=0x7a05c00, X=@0x7ffd945de090: 0x7a05fc0,
    CA=@0x7ffd945de088: 0x7a07888, CB=@0x7ffd945de080: 0x7a05af8)
    at ../lib/Analysis/ValueTracking.cpp:4376
#15 0x0000000002170ec0 in isTruePredicate (Pred=llvm::CmpInst::ICMP_ULE, LHS=0x7a078d0,
    RHS=0x7a05c00, DL=..., Depth=6) at ../lib/Analysis/ValueTracking.cpp:4387
#16 0x0000000002170c71 in isImpliedCondOperands (Pred=llvm::CmpInst::ICMP_ULT,
    ALHS=0x7a05c00, ARHS=0x7a05b10, BLHS=0x7a078d0, BRHS=0x7a05b10, DL=..., Depth=6)
    at ../lib/Analysis/ValueTracking.cpp:4414
#17 0x00000000021675c1 in isImpliedCondICmps (LHS=0x7a27d60, RHS=0x7a07960, DL=...,
    LHSIsTrue=true, Depth=6) at ../lib/Analysis/ValueTracking.cpp:4518
#18 0x000000000216720a in llvm::isImpliedCondition (LHS=0x7a27d60, RHS=0x7a07960,
    DL=..., LHSIsTrue=true, Depth=6) at ../lib/Analysis/ValueTracking.cpp:4581
#19 0x000000000216785a in isImpliedCondAndOr (LHS=0x7a27dd0, RHS=0x7a07960, DL=...,
    LHSIsTrue=true, Depth=5) at ../lib/Analysis/ValueTracking.cpp:4549
#20 0x0000000002167278 in llvm::isImpliedCondition (LHS=0x7a27dd0, RHS=0x7a07960,
    DL=..., LHSIsTrue=true, Depth=5) at ../lib/Analysis/ValueTracking.cpp:4589
#21 0x0000000002167798 in isImpliedCondAndOr (LHS=0x7a066b0, RHS=0x7a07960, DL=...,
    LHSIsTrue=true, Depth=4) at ../lib/Analysis/ValueTracking.cpp:4546
#22 0x0000000002167278 in llvm::isImpliedCondition (LHS=0x7a066b0, RHS=0x7a07960,

Diff Detail

Event Timeline

davide created this revision.Aug 9 2017, 5:05 AM
mcrosier accepted this revision.Aug 9 2017, 7:57 AM

One small comment, but otherwise LGTM. Thanks, Davide.

lib/Analysis/ValueTracking.cpp
4534

Is this check still necessary? I don't think it is, but of course, I'm the one who broke things in the first place.. :P

This revision is now accepted and ready to land.Aug 9 2017, 7:57 AM
davide added a comment.Aug 9 2017, 8:00 AM

One small comment, but otherwise LGTM. Thanks, Davide.

Thanks for your review.

lib/Analysis/ValueTracking.cpp
4534

I don't think so. I'd replace this with an assertion on trunk, but I'll keep it for safety when doing the backport to 5.

mcrosier added inline comments.Aug 9 2017, 8:12 AM
lib/Analysis/ValueTracking.cpp
4534

SGTM!

This revision was automatically updated to reflect the committed changes.