<html xmlns:v="urn:schemas-microsoft-com:vml" xmlns:o="urn:schemas-microsoft-com:office:office" xmlns:w="urn:schemas-microsoft-com:office:word" xmlns:m="http://schemas.microsoft.com/office/2004/12/omml" xmlns="http://www.w3.org/TR/REC-html40">
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
<meta name="Generator" content="Microsoft Word 15 (filtered medium)">
<style><!--
/* Font Definitions */
@font-face
{font-family:SimSun;
panose-1:2 1 6 0 3 1 1 1 1 1;}
@font-face
{font-family:"Cambria Math";
panose-1:2 4 5 3 5 4 6 3 2 4;}
@font-face
{font-family:Calibri;
panose-1:2 15 5 2 2 2 4 3 2 4;}
@font-face
{font-family:"\@SimSun";
panose-1:2 1 6 0 3 1 1 1 1 1;}
/* Style Definitions */
p.MsoNormal, li.MsoNormal, div.MsoNormal
{margin:0in;
margin-bottom:.0001pt;
font-size:12.0pt;
font-family:"Times New Roman",serif;}
a:link, span.MsoHyperlink
{mso-style-priority:99;
color:blue;
text-decoration:underline;}
a:visited, span.MsoHyperlinkFollowed
{mso-style-priority:99;
color:purple;
text-decoration:underline;}
span.EmailStyle17
{mso-style-type:personal-reply;
font-family:"Calibri",sans-serif;
color:#1F497D;}
.MsoChpDefault
{mso-style-type:export-only;
font-family:"Calibri",sans-serif;}
@page WordSection1
{size:8.5in 11.0in;
margin:1.0in 1.0in 1.0in 1.0in;}
div.WordSection1
{page:WordSection1;}
--></style><!--[if gte mso 9]><xml>
<o:shapedefaults v:ext="edit" spidmax="1026" />
</xml><![endif]--><!--[if gte mso 9]><xml>
<o:shapelayout v:ext="edit">
<o:idmap v:ext="edit" data="1" />
</o:shapelayout></xml><![endif]-->
</head>
<body lang="EN-US" link="blue" vlink="purple">
<div class="WordSection1">
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Hi Piotr,<o:p></o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"><o:p> </o:p></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?<o:p></o:p></span></p>
<p class="MsoNormal" style="text-indent:9.6pt"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"><o:p> </o:p></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.
<o:p></o:p></span></p>
<p class="MsoNormal" style="text-indent:9.6pt"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"><o:p> </o:p></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. <o:p></o:p></span></p>
<p class="MsoNormal" style="text-indent:9.6pt"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"><o:p> </o:p></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. 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.
<o:p></o:p></span></p>
<p class="MsoNormal"><a name="_MailEndCompose"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"><o:p> </o:p></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!<o:p></o:p></span></p>
<p class="MsoNormal" style="text-indent:9.6pt"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D"><o:p> </o:p></span></p>
<p class="MsoNormal"><span style="font-size:11.0pt;font-family:"Calibri",sans-serif;color:#1F497D">Regards,<br>
Ray<o:p></o:p></span></p>
<p class="MsoNormal"><a name="_____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:dberlin@dberlin.org]
<br>
<b>Sent:</b> Thursday, January 14, 2016 3:42 PM<br>
<b>To:</b> reviews+D16100+public+7f8d41cf7877574b@reviews.llvm.org<br>
<b>Cc:</b> Zhang, Yuanrui <yuanrui.zhang@intel.com>; Nick Lewycky <nlewycky@google.com>; Piotr Padlewski <piotr.padlewski@gmail.com>; Kreitzer, David L <david.l.kreitzer@intel.com>; llvm-commits <llvm-commits@lists.llvm.org><br>
<b>Subject:</b> Re: [PATCH] D16100: Fix for two constant propagation problems in GVN with assume intrinsic instruction<o:p></o:p></span></p>
<p class="MsoNormal"><o:p> </o:p></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).<o:p></o:p></p>
<div>
<p class="MsoNormal" style="margin-bottom:12.0pt"><o:p> </o:p></p>
</div>
</div>
<div>
<p class="MsoNormal"><o:p> </o:p></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:<o:p></o:p></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>
<o:p></o:p></p>
</blockquote>
</div>
<p class="MsoNormal"><o:p> </o:p></p>
</div>
</div>
</body>
</html>