On Monday, June 3, 2013, Rafael Espíndola  wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On 2 June 2013 04:08, David Majnemer <<a href="javascript:;" onclick="_e(event, 'cvml', 'david.majnemer@gmail.com')">david.majnemer@gmail.com</a>> wrote:<br>

> PR16069 is an interesting case where an incoming value to a PHI is a trap<br>
> value while also being a 'ConstantExpr'.<br>
><br>
> We do not consider this case when performing the 'HoistThenElseCodeToIf'<br>
> optimization.<br>
><br>
> The attached patch will make our modifications more conservative if we<br>
> detect that we cannot transform the PHI to a select.<br>
<br>
The change to isValueEqualityComparison is just a cleanup, right? If<br>
so, please commit that first as an independent patch.</blockquote><div><br></div><div>Will do.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
The comment<br>
  // If we get here, we can hoist at least one instruction<br>
<br>
is now out of date.</blockquote><div><br></div><div>Fair, I'll clean that up.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Why do you need the isa<ConstantExpr>? Can't the check be just<br>
<br>
     if (!isSafeToSpeculativelyExecute(BB1V) ||<br>
!isSafeToSpeculativelyExecute(BB2V))<br>
       return Changed;</blockquote><div><br></div><div>The reason why is for performance and correctness. I don't want to look at every instruction that can be fed into an incoming value for a phi. I just want to see the ones that can be executed *before* the phi node in the same BB as the phi. Which are these? ConstantExprs. FWIW, another part of SimplifyCFG operates in the exact same way.<span></span></div>
<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
LGTM with those changes.<br>
<br>
Cheers,<br>
Rafael<br>
</blockquote>