<div dir="ltr">Sigh.<div><div><span style="font-size:12.8px">Yes, i missed this part, i only looked at the basicaa part.</span></div></div><div><span style="font-size:12.8px">(The testcase didn't get committed either).</span><br></div><div><br></div><div>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.</div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jul 10, 2017 at 2:26 PM, Friedman, Eli via llvm-commits <span dir="ltr"><<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Comments inline.<div><div class="h5"><br>
<br>
On 7/10/2017 1:15 PM, Farhana Aleen via llvm-commits wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
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-pr<wbr>oject?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/D3447<wbr>8</a><br>
<br>
Modified:<br>
llvm/trunk/include/llvm/Analy<wbr>sis/BasicAliasAnalysis.h<br>
llvm/trunk/lib/Analysis/Basic<wbr>AliasAnalysis.cpp<br>
llvm/trunk/lib/Analysis/Value<wbr>Tracking.cpp<br>
<br>
Modified: llvm/trunk/include/llvm/Analys<wbr>is/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-pr<wbr>oject/llvm/trunk/include/llvm/<wbr>Analysis/BasicAliasAnalysis.h?<wbr>rev=307581&r1=307580&r2=<wbr>307581&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- llvm/trunk/include/llvm/Analys<wbr>is/BasicAliasAnalysis.h (original)<br>
+++ llvm/trunk/include/llvm/Analys<wbr>is/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>
+ 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/BasicA<wbr>liasAnalysis.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-pr<wbr>oject/llvm/trunk/lib/Analysis/<wbr>BasicAliasAnalysis.cpp?rev=<wbr>307581&r1=307580&r2=307581&<wbr>view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- llvm/trunk/lib/Analysis/BasicA<wbr>liasAnalysis.cpp (original)<br>
+++ llvm/trunk/lib/Analysis/BasicA<wbr>liasAnalysis.cpp Mon Jul 10 13:15:40 2017<br>
@@ -922,11 +922,11 @@ ModRefInfo BasicAAResult::getModRefInfo(<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::aliasSameBasePo<wbr>interGEPs(const GEPOperator *GEP1,<br>
+ uint64_t V1Size,<br>
+ const GEPOperator *GEP2,<br>
+ uint64_t V2Size,<br>
+ const DataLayout &DL) {<br>
assert(GEP1->getPointerOperand<wbr>()->stripPointerCastsAndBarrie<wbr>rs() ==<br>
GEP2->getPointerOperand()->st<wbr>ripPointerCastsAndBarriers() &&<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->getNumO<wbr>perands() - 1);<br>
Value *GEP2LastIdx = GEP2->getOperand(GEP2->getNumO<wbr>perands() - 1);<br>
- if (isa<PHINode>(GEP1LastIdx) || isa<PHINode>(GEP2LastIdx)) {<br>
+ if ((isa<PHINode>(GEP1LastIdx) || isa<PHINode>(GEP2LastIdx)) &&<br>
+ !VisitedPhiBBs.empty()) {<br>
</blockquote>
<br></div></div>
No testcase for this change?<span class=""><br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
// 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/ValueT<wbr>racking.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-pr<wbr>oject/llvm/trunk/lib/Analysis/<wbr>ValueTracking.cpp?rev=307581&<wbr>r1=307580&r2=307581&view=diff</a><br>
==============================<wbr>==============================<wbr>==================<br>
--- llvm/trunk/lib/Analysis/ValueT<wbr>racking.cpp (original)<br>
+++ llvm/trunk/lib/Analysis/ValueT<wbr>racking.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>
</blockquote>
<br></span>
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.<br>
<br>
-Eli<span class="HOEnZb"><font color="#888888"><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</font></span><div class="HOEnZb"><div class="h5"><br>
<br>
______________________________<wbr>_________________<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/<wbr>mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div>