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