<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">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><div>Is that because we only use CxtI in icmp-related queries like computeKnownBitsFromAssume?<br></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>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><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_1511878800344373864gmail-HOEnZb"><div class="m_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></div><br></div></div>