[llvm] r307581 - Avoid doing conservative phi checks in aliasSameBasePointerGEPs() if no phis have been visited yet.
Daniel Berlin via llvm-commits
llvm-commits at lists.llvm.org
Mon Jul 10 20:57:24 PDT 2017
Sigh.
Yes, i missed this part, i only looked at the basicaa part.
(The testcase didn't get committed either).
As Eli says, this is pretty clearly not correct. It also should have been
split out from the basicaa changes if you want to take another shot at it,
because, among other things, i'm not really comfortable approving value
tracking changes.
On Mon, Jul 10, 2017 at 2:26 PM, 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/20170710/250df036/attachment.html>
More information about the llvm-commits
mailing list