<div dir="ltr">Sorry guys, I had a lot of things to do and I forgot about it. <div>I don't think it makes lot of sense to refactor GVN right now - Daniel is working on new GVN, I don't know how close is he. But it's true that the GVN we have right now is a little awful.</div><div><br></div><div>The thing that could fix issue with propageEquality, is that the code for propagation by the block should be simpler than the general code that we have here, but I don't know how much code would it have in common. </div><div><br></div><div>I don't have much time right now to think about it, so I hope you will find the way to fix it.</div><div>You can also ask Nick Lewycky for help.</div><div><br></div><div>Piotr</div></div><div class="gmail_extra"><br><div class="gmail_quote">2016-01-20 22:01 GMT+01:00 Daniel Berlin <span dir="ltr"><<a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a>></span>:<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 class="">On Wed, Jan 20, 2016 at 12:59 PM, Zhang, Yuanrui <span dir="ltr"><<a href="mailto:yuanrui.zhang@intel.com" target="_blank">yuanrui.zhang@intel.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div lang="EN-US" link="blue" vlink="purple">
<div>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d">Hi Piotr,<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal" style="text-indent:9.6pt"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d">Did you get a time to look at the fix I uploaded for the second time? Does it look good to you?<u></u><u></u></span></p>
<p class="MsoNormal" style="text-indent:9.6pt"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d"> Daniel’s suggestion on restructuring the propagateEquality is good and necessary, since many developers find this function confusing.
<u></u><u></u></span></p>
<p class="MsoNormal" style="text-indent:9.6pt"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal" style="text-indent:9.6pt"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d">Dave and I tried the idea he mentioned below, i.e., the new interface, but it seems that it does not help much to eliminate the code
confusion in that function, and not simpler than the current fix.</span></p></div></div></blockquote><div><br></div></span><div>Sigh.</div><div><br></div><div>Okay, i think then we should just punt on it for now. Once we push post-dominators through the stack, we'll just rewrite it to do this the traditional post-dominance way.</div><span class=""><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div lang="EN-US" link="blue" vlink="purple"><div><p class="MsoNormal" style="text-indent:9.6pt"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d"> <u></u><u></u></span></p>
<p class="MsoNormal" style="text-indent:9.6pt"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal" style="text-indent:9.6pt"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d">I am wondering if you have any plan to rewrite this function. </span></p></div></div></blockquote><div><br></div></span><div>I believe I am the only person who is trying to clean up GVN, and i'm doing it by rewriting it entirely :P</div><div>. </div><span class=""><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div lang="EN-US" link="blue" vlink="purple"><div><p class="MsoNormal" style="text-indent:9.6pt"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d">Are you familiar with the code? If not, do you know who is familiar with
this code, and can assist us in doing the rewriting. We can all help if needed, but we need an expert first.
<u></u><u></u></span></p>
<p class="MsoNormal"><a name="345734260_-1887064849__MailEndCompose"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d"><u></u> <u></u></span></a></p>
<p class="MsoNormal" style="text-indent:9.6pt"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d">Thank you very much!<u></u><u></u></span></p>
<p class="MsoNormal" style="text-indent:9.6pt"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1f497d">Regards,<br>
Ray<u></u><u></u></span></p>
<p class="MsoNormal"><a name="345734260_-1887064849______replyseparator"></a><b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif">From:</span></b><span style="font-size:11.0pt;font-family:"Calibri",sans-serif"> Daniel Berlin [mailto:<a href="mailto:dberlin@dberlin.org" target="_blank">dberlin@dberlin.org</a>]
<br>
<b>Sent:</b> Thursday, January 14, 2016 3:42 PM<br>
<b>To:</b> <a href="mailto:reviews%2BD16100%2Bpublic%2B7f8d41cf7877574b@reviews.llvm.org" target="_blank">reviews+D16100+public+7f8d41cf7877574b@reviews.llvm.org</a><br>
<b>Cc:</b> Zhang, Yuanrui <<a href="mailto:yuanrui.zhang@intel.com" target="_blank">yuanrui.zhang@intel.com</a>>; Nick Lewycky <<a href="mailto:nlewycky@google.com" target="_blank">nlewycky@google.com</a>>; Piotr Padlewski <<a href="mailto:piotr.padlewski@gmail.com" target="_blank">piotr.padlewski@gmail.com</a>>; Kreitzer, David L <<a href="mailto:david.l.kreitzer@intel.com" target="_blank">david.l.kreitzer@intel.com</a>>; llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>><br>
<b>Subject:</b> Re: [PATCH] D16100: Fix for two constant propagation problems in GVN with assume intrinsic instruction<u></u><u></u></span></p><div><div>
<p class="MsoNormal"><u></u> <u></u></p>
<div>
<p class="MsoNormal">To start with, I'm fine with committing this patch if we agree to address the other issues, *somehow*, in a followup (either by y'all or by Piotr).<u></u><u></u></p>
<div>
<p class="MsoNormal" style="margin-bottom:12.0pt"><u></u> <u></u></p>
</div>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
<div>
<p class="MsoNormal">On Thu, Jan 14, 2016 at 3:31 PM, David Kreitzer <<a href="mailto:david.l.kreitzer@intel.com" target="_blank">david.l.kreitzer@intel.com</a>> wrote:<u></u><u></u></p>
<blockquote style="border:none;border-left:solid #cccccc 1.0pt;padding:0in 0in 0in 6.0pt;margin-left:4.8pt;margin-right:0in">
<p class="MsoNormal" style="margin-bottom:12.0pt">DavidKreitzer added a comment.<br>
<br>
I like Ray's plan of first making the targeted stability fix and then deciding how best to restructure propagateEquality to make it clearer. The bug illustrated by the @_Z1im test case is pretty egregious and ought to be fixed promptly IMO.<br>
<br>
Daniel, you suggested possibly splitting propagateEquality, but there is a lot of functionality in there that you'd like to share between the assume intrinsic caller and the other callers. For example, the code that infers A == 1 && B == 1 from A && B == 1
applies equally regardless of whether A && B == 1 came from "assume(A && B)" or "if (A && B)".<br>
<br>
I think the fundamental difference between assume and the other callers of propagateEquality is that for assume, the equality property becomes known true immediately following the assume instruction while for the other callers, the equality property becomes
known true along the outgoing edge of the block ending in the conditional branch or switch. It shouldn't be hard to make that distinction in the interface to propagateEquality. Perhaps something like this:<br>
<br>
/// The given values are known to be equal at a certain point, P, in the program.<br>
/// When DominatesByEdge is true, that point is the CFG edge Root.<br>
/// When DominatesByEdge is false, that point is the Instruction Instr.<br>
/// Exploit this, for example by replacing 'LHS' with 'RHS' at all uses dominated by P.<br>
/// Returns whether a change was made.<br>
bool GVN::propagateEquality(Value *LHS, Value *RHS, const BasicBlockEdge &Root,<br>
Instruction *Instr, bool DominatesByEdge) {<br>
<br>
When DominatesByEdge is true, Instr would be ignored. And when DominatesByEdge is false, Root would be ignored. Then the dominance check for the assume case becomes one of "does this instruction dominate" rather than "does this block dominate". (Note that this
lets us get rid of the "for each successor" loop inside processAssumeIntrinsic, which is pretty artificial. I suspect it was written that way just to make it easier to reuse propagateEquality.)<br>
<br>
<br>
<a href="http://reviews.llvm.org/D16100" target="_blank">http://reviews.llvm.org/D16100</a><br>
<br>
<br>
<u></u><u></u></p>
</blockquote>
</div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
</div></div></div>
</div>
</blockquote></span></div><br></div></div>
</blockquote></div><br></div>