[llvm] r307581 - Avoid doing conservative phi checks in aliasSameBasePointerGEPs() if no phis have been visited yet.

[Friedman, Eli via llvm-commits]

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