<div dir="ltr">Sounds good. Thanks for the explanation, Hal. <div><br></div><div>So, do you think this patch is good to go then? </div></div><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Jun 25, 2015 at 11:43 AM, Hal Finkel <span dir="ltr"><<a href="mailto:hfinkel@anl.gov" target="_blank">hfinkel@anl.gov</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">----- Original Message -----<br>
> From: "David Majnemer" <<a href="mailto:david.majnemer@gmail.com">david.majnemer@gmail.com</a>><br>
> To: "Hal Finkel" <<a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a>><br>
</span><span class="">> Cc: "Jingyue Wu" <<a href="mailto:jingyue@google.com">jingyue@google.com</a>>, <a href="mailto:reviews%2BD10695%2Bpublic%2B1cd92e3c8232918a@reviews.llvm.org">reviews+D10695+public+1cd92e3c8232918a@reviews.llvm.org</a>, "henry hu sh"<br>
> <<a href="mailto:henry.hu.sh@gmail.com">henry.hu.sh@gmail.com</a>>, "LLVM Commits" <<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a>>, "Xuetian Weng" <<a href="mailto:xweng@google.com">xweng@google.com</a>>, "Bjarke<br>
> Roune" <<a href="mailto:broune@google.com">broune@google.com</a>>, "Mark Heffernan" <<a href="mailto:meheff@google.com">meheff@google.com</a>>, "Eli Bendersky" <<a href="mailto:eliben@google.com">eliben@google.com</a>><br>
</span><span class="">> Sent: Thursday, June 25, 2015 1:34:27 PM<br>
> Subject: Re: [PATCH] [InstCombine] call SimplifyICmpInst with correct context<br>
><br>
><br>
><br>
><br>
><br>
><br>
</span><span class="">> On Thu, Jun 25, 2015 at 11:29 AM, David Majnemer <<br>
> <a href="mailto:david.majnemer@gmail.com">david.majnemer@gmail.com</a> > wrote:<br>
><br>
><br>
><br>
><br>
><br>
><br>
><br>
> On Thu, Jun 25, 2015 at 11:22 AM, Hal Finkel < <a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a> ><br>
> wrote:<br>
><br>
><br>
> ----- Original Message -----<br>
> > From: "Jingyue Wu" < <a href="mailto:jingyue@google.com">jingyue@google.com</a> ><br>
> > To: "David Majnemer" < <a href="mailto:david.majnemer@gmail.com">david.majnemer@gmail.com</a> ><br>
</span><span class="">> > Cc: <a href="mailto:reviews%2BD10695%2Bpublic%2B1cd92e3c8232918a@reviews.llvm.org">reviews+D10695+public+1cd92e3c8232918a@reviews.llvm.org</a> , "Hal<br>
> > Finkel" < <a href="mailto:hfinkel@anl.gov">hfinkel@anl.gov</a> >, "henry hu sh"<br>
> > < <a href="mailto:henry.hu.sh@gmail.com">henry.hu.sh@gmail.com</a> >, "LLVM Commits" <<br>
> > <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a> >, "Xuetian Weng" < <a href="mailto:xweng@google.com">xweng@google.com</a> >,<br>
> > "Bjarke<br>
> > Roune" < <a href="mailto:broune@google.com">broune@google.com</a> >, "Mark Heffernan" < <a href="mailto:meheff@google.com">meheff@google.com</a><br>
> > >, "Eli Bendersky" < <a href="mailto:eliben@google.com">eliben@google.com</a> ><br>
> > Sent: Thursday, June 25, 2015 12:56:13 PM<br>
> > Subject: Re: [PATCH] [InstCombine] call SimplifyICmpInst with<br>
> > correct context<br>
> ><br>
> ><br>
> > Agreed. Let me see how the correctness issue can be fixed.<br>
> ><br>
><br>
> I'm missing something. What's the correctness issue?<br>
><br>
><br>
><br>
> My concern is that there is no way to communicate that no assume<br>
> context is appropriate for analyzing an instruction.<br>
><br>
><br>
> However, maybe this isn't problematic. I couldn't come up with a<br>
> plausible scenario where we'd want to run without a context because<br>
> doing so might result in miscompiles and the only harm in inferring<br>
> a context is that we might nuke an assume earlier than we might've.<br>
<br>
</span>I'm fairly certain that there isn't one. If there is, then I made a mistake somewhere.<br>
<br>
 -Hal<br>
<div class="HOEnZb"><div class="h5"><br>
><br>
><br>
> -Hal<br>
><br>
><br>
><br>
> ><br>
> > On Wed, Jun 24, 2015 at 10:23 PM, David Majnemer <<br>
> > <a href="mailto:david.majnemer@gmail.com">david.majnemer@gmail.com</a> > wrote:<br>
> ><br>
> ><br>
> > On Wed, Jun 24, 2015 at 10:00 PM, Jingyue Wu < <a href="mailto:jingyue@google.com">jingyue@google.com</a> ><br>
> > wrote:<br>
> ><br>
> ><br>
> > And just to be clear, I agree with you that it's a potential issue<br>
> > that `ComputeSignBit` is unable to say "I don't have a context".<br>
> > But<br>
> > my concern is, even if we fix that issue, not passing the context<br>
> > instruction to `SimplifyICmpInst` at all in `InstCombine` may<br>
> > overkill some useful optimizations.<br>
> ><br>
> ><br>
> ><br>
> > I see these as two separable issues: a correctness issue and a<br>
> > performance issue. My thinking was that we should solve the<br>
> > correctness issue by being conservative at the API boundary and<br>
> > regain lost performance by providing the context instruction. My<br>
> > fear is not that your change to SimplifyICmpInst is incorrect but<br>
> > that all the other callers of the various SimplifyXYZInst API have<br>
> > latent bugs because they aren't passing in a context.<br>
> ><br>
> ><br>
> ><br>
> > Btw, I noticed that `SimplifyDemandedBits` indeed pass `UserI` as<br>
> > the<br>
> > context instruction (<br>
> > <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_docs_doxygen_html_InstCombineSimplifyDemanded-5F8cpp-5Fsource.html-23l00075&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=9sqTZ-qAqcd3XBSlEx2l6pCSHNnO4tn6YsEzDhl7JBQ&s=Q4LGIZjXN1pV44p56qVQshXB4vkSGB04VXDGMVdATaQ&e=" rel="noreferrer" target="_blank">http://llvm.org/docs/doxygen/html/InstCombineSimplifyDemanded_8cpp_source.html#l00075</a><br>
> > ), which seems aligned with the approach in my patch.<br>
> ><br>
> ><br>
> ><br>
> ><br>
> > <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D10695&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=9sqTZ-qAqcd3XBSlEx2l6pCSHNnO4tn6YsEzDhl7JBQ&s=UuxKkzTpl0gJhkfKRlZ_z0sOxn_kXwCy9UuMe7zQaF4&e=" rel="noreferrer" target="_blank">http://reviews.llvm.org/D10695</a><br>
> ><br>
> > EMAIL PREFERENCES<br>
> > <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_settings_panel_emailpreferences_&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=9sqTZ-qAqcd3XBSlEx2l6pCSHNnO4tn6YsEzDhl7JBQ&s=GlS-_SdZ1-weP5C_Iq1eAm99IOgXXIJcgfChohJ6jb0&e=" rel="noreferrer" target="_blank">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br>
> ><br>
> ><br>
> ><br>
> ><br>
> ><br>
><br>
> --<br>
> Hal Finkel<br>
> Assistant Computational Scientist<br>
> Leadership Computing Facility<br>
> Argonne National Laboratory<br>
><br>
><br>
><br>
<br>
--<br>
Hal Finkel<br>
Assistant Computational Scientist<br>
Leadership Computing Facility<br>
Argonne National Laboratory<br>
</div></div></blockquote></div><br></div>