<div dir="ltr"><div class="gmail_quote"><div>FWIW: Before you take the absolutist position you have taken, you may want to consider the below.<br></div><div><br></div><div> </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> The output of the PRE/FRE here is a phi which looks like this:<br>
merge:                                            ; preds = %bb1, %bb<br>
<br>
  %foo = phi i32 [ %add.2, %bb1 ], [ %add.1, %bb ]<br>
<br>
That's correct.<br></blockquote><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> The only reason it determines the adds are equivalent is because it ignores nsw during value numbering. Otherwise, it would not believe they were equivalent.<br>
<br>
Danny, this is where I completely loose you.  As far as I can tell, GVN/PRE is not eliminating the phi.  The output appears to be completely correct.  Can you clarify here?<br></blockquote><div><br></div><div>GVN is not currently smart enough to eliminate an add using the phi in most cases, but that's just basically "a feature request away".</div><div>Y'all are basically saying "let's not fix this until we have a bug report". That seems silly to me in this case<br></div><div>It's 100% guaranteed to get the wrong answer at that time without any further intervention. There is nothing in this code, no testcase, no nothing, that will cause someone to notice that.</div><div>Unless you or I or Eli review it, and remember back to this email thread, it's going to generate broken code.</div><div>You are guaranteed anyone who makes it smart enough to see the phi and the add are the same (and they are according to the way it value numbers ignoring flags) will cause a hard to notice bug (this one took years).</div><div><br></div><div>Right now it will detect some of the original cases in Scalar PRE and will get them mostly right (but not for lack of trying).  It will generate IR that someone will want to obviously improve:</div><div><div> %foo = load i32, i32* @G, align 4</div><div>  %bar.2 = add i32 %v, -1</div><div>  %cmp = icmp sgt i32 %foo, 0</div><div>  %cmp2 = icmp sgt i32 %bar.2, 0</div><div>  store i1 %cmp2, i1* @I, align 4</div><div>  br i1 %cmp, label %action, label %return</div></div><div>-><br><div>  %bar.2.pre-phi = phi i32 [ %add.2, %bb1 ], [ %add.1, %bb ]</div><div>  %foo = phi i32 [ %add.2, %bb1 ], [ %add.1, %bb ]</div><div>  %cmp = icmp sgt i32 %foo, 0</div><div>  %cmp2 = icmp sgt i32 %bar.2.pre-phi, 0</div><div>  store i1 %cmp2, i1* @I, align 4</div><div>  br i1 %cmp, label %action, label %return</div></div><div><br></div><div>If this goes through any path that isn't scalar PRE, GVN would get this wrong. </div><div>It isn't a PRE problem, and PRE generates crappy and redundant code for it, so eventually, the likelihood that happens seems to be to be nearly 100%</div><div><br></div><div>At the absolute least a comment should be added.</div><div>In practice, we should either fix this or add a testcase that will fail when the day comes that someone improves GVN.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">The phi is entirely correct and could be hand written.</blockquote><div><br></div><div>Sure, but that's a red herring. You can hand write code with any semantics you want. </div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">  If GVN is considering adds with different flags equivalent without appropriate backpatching, that's a bug in GVN's replacement.  End of story.<br></blockquote><div><br></div><div>I am suggesting that rather than add O(N) recursion to backpatching, to ensure that everything in the def-use chain had it's flags changed, that  we do what we are doing in the scalar PRE case: fixing it when we add the phi node. </div><div>We in fact, don't rely on backpatching to fix it there because it would be O(N) to do it at backpatching time.</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
</blockquote></div></div>