<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"><br clear="all"><div><div class="gmail_signature" data-smartmail="gmail_signature">~Craig</div></div>
<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="HOEnZb"><div class="h5"><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_7829809801032759398m_-1329168261770779379m_6254560931461075387m_1511878800344373864gmail-HOEnZb"><div class="m_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>