<div dir="ltr">IMO - yes. We're relying on an implementation detail of computeKnownBits when deciding how to call it. If consistency is the goal, then we should always pass in the CxtI from InstCombine because we obviously have it.<br></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, May 26, 2017 at 12:20 PM, Craig Topper <span dir="ltr"><<a href="mailto:craig.topper@gmail.com" target="_blank">craig.topper@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">But all the calls to computeKnownBits made in non icmp/fcmp code pass the context inst. Shouldn't we be consistent by giving it to the commuteKnownBits calls InstSimplify might do as well?</div><div class="gmail_extra"><span class="HOEnZb"><font color="#888888"><br clear="all"><div><div class="m_-8293404662271756486gmail_signature" data-smartmail="gmail_signature">~Craig</div></div></font></span><div><div class="h5">
<br><div class="gmail_quote">On Fri, May 26, 2017 at 11:17 AM, Sanjay Patel <span dir="ltr"><<a href="mailto:spatel@rotateright.com" target="_blank">spatel@rotateright.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">Aha - so I think that answers my initial question: we only pass the context inst in instcombine from visitICmp/visitFCmp because those are the only ones that can benefit.<br></div><div class="m_-8293404662271756486HOEnZb"><div class="m_-8293404662271756486h5"><div class="gmail_extra"><br><div class="gmail_quote">On Fri, May 26, 2017 at 12:06 PM, Daniel Berlin <span dir="ltr"><<a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Fri, May 26, 2017 at 10:54 AM, Sanjay Patel <span dir="ltr"><<a href="mailto:spatel@rotateright.com" target="_blank">spatel@rotateright.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote"><span>On Fri, May 26, 2017 at 11:13 AM, Daniel Berlin <span dir="ltr"><<a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">I tried to make sure we passed the right instruction in each case, but i may have screwed up :)<div>note that:</div><div><span style="font-size:12.8px"> SQ.getWithInstruction(&I)</span><br></div><div><span style="font-size:12.8px">vs</span></div><div><span style="font-size:12.8px"> SQ will give different results if the *context instruction* is not I.</span><span style="font-size:12.8px"><br></span></div><div><br></div></div></blockquote><div><br></div></span><div>Is that because we only use CxtI in icmp-related queries like computeKnownBitsFromAssume?<br></div></div></div></div></blockquote></span><div>Yes</div><span><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div></div><div>Ie, when we call Simplify*Inst with the plain 'SQ' in instcombine, CxtI is 'null' in that query. There is nothing filling that in with the default 'I' from what I see.<br><br></div></div></div></div></blockquote></span><div>We fill it in from SimplifyInstruction, but not other things, because we don't have it :)</div><span><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>If the simplify routine uses computeKnownBits or some other value tracking, then there could be a different result if known bits was somehow using CxtI for non-icmp analysis?<br></div></div></div></div></blockquote></span><div><br>I believe, at the moment, it will only affect assumes.</div><div>At lesat, when i screwed it up, that was the only testcase that failed.</div><span><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div></div><span><div><br> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div></div><div>I swear there was a case in instcombine where this the case originally, and i made sure it continues.</div></div><div class="m_-8293404662271756486m_7829809801032759398m_-1329168261770779379m_6254560931461075387m_1511878800344373864gmail-HOEnZb"><div class="m_-8293404662271756486m_7829809801032759398m_-1329168261770779379m_6254560931461075387m_1511878800344373864gmail-h5"><div class="gmail_extra"><br><div class="gmail_quote">On Fri, May 26, 2017 at 10:06 AM, Sanjay Patel via Phabricator <span dir="ltr"><<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">spatel added subscribers: davide, hfinkel, dberlin.<br>
spatel added a comment.<br>
<br>
Can you concoct tests with 'llvm.assume' to show a difference?<br>
<br>
I think that raises a question though: this change can make value tracking more effective, but it comes with non-zero compile-time cost if I understand computeKnownBits() correctly. Do we want to incur that overhead without a real-world motivation for the benefit?<br>
cc'ing @dberlin @davide @hfinkel<br>
<br>
Another question along this line: we call Simplify*Inst() for all instructions as the first step in every visit*() in InstCombine. In most cases, we don't pass a context instruction, but we do for visitICmpInst(). Is that difference intentional?<br>
<br>
if (Value *V = SimplifyICmpInst(I.getPredicat<wbr>e(), Op0, Op1,<br>
SQ.getWithInstruction(&I)))<br>
return replaceInstUsesWith(I, V);<br>
<br>
vs.<br>
<br>
if (Value *V = SimplifyShlInst(Op0, Op1, I.hasNoSignedWrap(),<br>
I.hasNoUnsignedWrap(), SQ))<br>
return replaceInstUsesWith(I, V);<br>
<br>
<br>
<a href="https://reviews.llvm.org/D33567" rel="noreferrer" target="_blank">https://reviews.llvm.org/D3356<wbr>7</a><br>
<br>
<br>
<br>
</blockquote></div><br></div>
</div></div></blockquote></span></div><br></div></div>
</blockquote></span></div><br></div></div>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div></div></div>
</blockquote></div><br></div>