[llvm] r307581 - Avoid doing conservative phi checks in aliasSameBasePointerGEPs() if no phis have been visited yet.
NAKAMURA Takumi via llvm-commits
llvm-commits at lists.llvm.org
Mon Jul 10 20:38:26 PDT 2017
It broke selfhosting. Reverted in r307613.
On Tue, Jul 11, 2017 at 6:26 AM Friedman, Eli via llvm-commits <
llvm-commits at lists.llvm.org> wrote:
> Comments inline.
>
> On 7/10/2017 1:15 PM, Farhana Aleen via llvm-commits wrote:
> > Author: faaleen
> > Date: Mon Jul 10 13:15:40 2017
> > New Revision: 307581
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=307581&view=rev
> > Log:
> > Avoid doing conservative phi checks in aliasSameBasePointerGEPs() if no
> phis have been visited yet.
> >
> > Reviewers: Daniel Berlin
> >
> > Subscribers: llvm-commits
> >
> > Differential Revision: https://reviews.llvm.org/D34478
> >
> > Modified:
> > llvm/trunk/include/llvm/Analysis/BasicAliasAnalysis.h
> > llvm/trunk/lib/Analysis/BasicAliasAnalysis.cpp
> > llvm/trunk/lib/Analysis/ValueTracking.cpp
> >
> > Modified: llvm/trunk/include/llvm/Analysis/BasicAliasAnalysis.h
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/Analysis/BasicAliasAnalysis.h?rev=307581&r1=307580&r2=307581&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/include/llvm/Analysis/BasicAliasAnalysis.h (original)
> > +++ llvm/trunk/include/llvm/Analysis/BasicAliasAnalysis.h Mon Jul 10
> 13:15:40 2017
> > @@ -183,6 +183,12 @@ private:
> > uint64_t V2Size, const AAMDNodes &V2AAInfo,
> > const Value *UnderlyingV1, const Value
> *UnderlyingV2);
> >
> > + AliasResult aliasSameBasePointerGEPs(const GEPOperator *GEP1,
> > + uint64_t V1Size,
> > + const GEPOperator *GEP2,
> > + uint64_t V2Size,
> > + const DataLayout &DL);
> > +
> > AliasResult aliasPHI(const PHINode *PN, uint64_t PNSize,
> > const AAMDNodes &PNAAInfo, const Value *V2,
> > uint64_t V2Size, const AAMDNodes &V2AAInfo,
> >
> > Modified: llvm/trunk/lib/Analysis/BasicAliasAnalysis.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/BasicAliasAnalysis.cpp?rev=307581&r1=307580&r2=307581&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/lib/Analysis/BasicAliasAnalysis.cpp (original)
> > +++ llvm/trunk/lib/Analysis/BasicAliasAnalysis.cpp Mon Jul 10 13:15:40
> 2017
> > @@ -922,11 +922,11 @@ ModRefInfo BasicAAResult::getModRefInfo(
> >
> > /// Provide ad-hoc rules to disambiguate accesses through two GEP
> operators,
> > /// both having the exact same pointer operand.
> > -static AliasResult aliasSameBasePointerGEPs(const GEPOperator *GEP1,
> > - uint64_t V1Size,
> > - const GEPOperator *GEP2,
> > - uint64_t V2Size,
> > - const DataLayout &DL) {
> > +AliasResult BasicAAResult::aliasSameBasePointerGEPs(const GEPOperator
> *GEP1,
> > + uint64_t V1Size,
> > + const GEPOperator
> *GEP2,
> > + uint64_t V2Size,
> > + const DataLayout
> &DL) {
> >
> > assert(GEP1->getPointerOperand()->stripPointerCastsAndBarriers() ==
> > GEP2->getPointerOperand()->stripPointerCastsAndBarriers()
> &&
> > @@ -1006,7 +1006,7 @@ static AliasResult aliasSameBasePointerG
> > // Because they cannot partially overlap and because fields in an
> array
> > // cannot overlap, if we can prove the final indices are different
> between
> > // GEP1 and GEP2, we can conclude GEP1 and GEP2 don't alias.
> > -
> > +
> > // If the last indices are constants, we've already checked they
> don't
> > // equal each other so we can exit early.
> > if (C1 && C2)
> > @@ -1014,11 +1014,15 @@ static AliasResult aliasSameBasePointerG
> > {
> > Value *GEP1LastIdx = GEP1->getOperand(GEP1->getNumOperands() -
> 1);
> > Value *GEP2LastIdx = GEP2->getOperand(GEP2->getNumOperands() -
> 1);
> > - if (isa<PHINode>(GEP1LastIdx) || isa<PHINode>(GEP2LastIdx)) {
> > + if ((isa<PHINode>(GEP1LastIdx) || isa<PHINode>(GEP2LastIdx)) &&
> > + !VisitedPhiBBs.empty()) {
>
> No testcase for this change?
> > // If one of the indices is a PHI node, be safe and only use
> > // computeKnownBits so we don't make any assumptions about the
> > // relationships between the two indices. This is important if
> we're
> > // asking about values from different loop iterations. See
> PR32314.
> > + // But, with empty visitedPhiBBs we can guarantee that the
> values are
> > + // from the same iteration. Therefore, we can avoid doing this
> > + // conservative check.
> > // TODO: We may be able to change the check so we only do this
> when
> > // we definitely looked through a PHINode.
> > if (GEP1LastIdx != GEP2LastIdx &&
> >
> > Modified: llvm/trunk/lib/Analysis/ValueTracking.cpp
> > URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/ValueTracking.cpp?rev=307581&r1=307580&r2=307581&view=diff
> >
> ==============================================================================
> > --- llvm/trunk/lib/Analysis/ValueTracking.cpp (original)
> > +++ llvm/trunk/lib/Analysis/ValueTracking.cpp Mon Jul 10 13:15:40 2017
> > @@ -1873,7 +1873,7 @@ bool isKnownNonZero(const Value *V, unsi
> > if (Known.countMaxLeadingZeros() < BitWidth - ShiftVal)
> > return true;
> > // Are all the bits to be shifted out known zero?
> > - if (Known.countMinTrailingZeros() >= ShiftVal)
> > + if (Known.isUnknown() || Known.countMinTrailingZeros() >=
> ShiftVal)
> > return isKnownNonZero(X, Depth, Q);
> > }
> > }
> >
>
> This is pretty clearly wrong. Consider, for example, "(1 << x) >> 5":
> according to your logic, this is non-zero because "1 << x" doesn't have
> any known bits.
>
> -Eli
>
> --
> Employee of Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux
> Foundation Collaborative Project
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170711/c41d38c7/attachment.html>
More information about the llvm-commits
mailing list