<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br><div><div>On Aug 13, 2013, at 3:33 PM, Eli Friedman <<a href="mailto:eli.friedman@gmail.com">eli.friedman@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div dir="ltr">On Tue, Aug 13, 2013 at 3:09 PM, Jim Grosbach <span dir="ltr"><<a href="mailto:grosbach@apple.com" target="_blank">grosbach@apple.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><br><div><div><div class="h5"><div>On Aug 13, 2013, at 2:39 PM, Eli Friedman <<a href="mailto:eli.friedman@gmail.com" target="_blank">eli.friedman@gmail.com</a>> wrote:</div>
<br><blockquote type="cite"><div dir="ltr">On Tue, Aug 13, 2013 at 2:30 PM, Jim Grosbach <span dir="ltr"><<a href="mailto:grosbach@apple.com" target="_blank">grosbach@apple.com</a>></span> wrote:<br><div class="gmail_extra">
<div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Author: grosbach<br>
Date: Tue Aug 13 16:30:58 2013<br>
New Revision: 188315<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=188315&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=188315&view=rev</a><br>
Log:<br>
DAG: Combine (and (setne X, 0), (setne X, -1)) -> (setuge (add X, 1), 2)<br>
<br>
A common idiom is to use zero and all-ones as sentinal values and to<br>
check for both in a single conditional ("x != 0 && x != (unsigned)-1").<br>
That generates code, for i32, like:<br>
testl %edi, %edi<br>
setne %al<br>
cmpl $-1, %edi<br>
setne %cl<br>
andb %al, %cl<br>
<br>
With this transform, we generate the simpler:<br>
incl %edi<br>
cmpl $1, %edi<br>
seta %al<br>
<br>
Similar improvements for other integer sizes and on other platforms. In<br>
general, combining the two setcc instructions into one is better.<br>
<br>
<a>rdar://14689217</a><br><br></blockquote><div><br></div><div>We already have code in InstCombiner::FoldAndOfICmps to handle this sort of thing; it looks like it isn't catching this particular case for some reason, though.</div>
<div><br></div></div></div></div></blockquote><div><br></div></div></div><div>There’s already a bunch of similar checks in the DAGCombiner, too. I mainly put it there to that we’ll be able to catch more complicate cases that simplify to this one from other DAG transformations, that are exposed from target lowerings, etc..</div>
</div></div></blockquote><div><br></div><div>I'm not convinced it will actually show up from lowering in practice. </div></div></div></div></blockquote><div><br></div><div>I’m not convinced either way, really. Simply a mild motivation in one direction over the other. *shrug*</div><div><br></div><div>That said, then why do we have any of the transformations we do for setcc in DAGCombine? The same sort of arguments can be made equally strongly for them, yes?</div></div><div><br><blockquote type="cite" dir="auto"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><div>Also, the transformation depends on the wrapping behavior of the add. While we can express that in IR, it feels a bit more appropriate to do that sort of change at the DAG level.</div>
</div></blockquote><div><br></div><div>I don't follow. Addition has the same semantics in IR and SelectionDAG.</div></div></div></div></blockquote><div><br></div><div>The wrapping is exactly why this case isn’t caught by the current InstCombine code, FWIW. That doesn’t mean it shouldn’t be, of course. Just that the current code seems to be being pretty careful to not rely on wrapping, and I’m hesitant to perturb that.</div><div><br></div><blockquote type="cite" dir="auto"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div style="word-wrap:break-word"><div>Do either of you have a strong preference for InstCombine instead? This isn’t the sort of thing where we have a definitive “right place” to put the transform, really.</div><span class="HOEnZb"><font color="#888888">
</font></span></div></blockquote></div><br></div><div class="gmail_extra">Considering we already have code which catches "(x != 0) & (x != 1)", we should probably catch "(x != 0) & (x != -1)" there as well.</div></div></blockquote><div><br></div>This makes a lot of sense, however.</div><div><br></div><div>Assuming we do add this to InstCombine, the question becomes whether to keep it in both places.</div><div><br></div><div>I’ll ponder a bit more.</div><div><br></div><div>-Jim</div></body></html>