[llvm] r188315 - DAG: Combine (and (setne X, 0), (setne X, -1)) -> (setuge (add X, 1), 2)

[Jim Grosbach]

On Aug 13, 2013, at 3:33 PM, Eli Friedman <eli.friedman at gmail.com> wrote:

> On Tue, Aug 13, 2013 at 3:09 PM, Jim Grosbach <grosbach at apple.com> wrote:
> 
> On Aug 13, 2013, at 2:39 PM, Eli Friedman <eli.friedman at gmail.com> wrote:
> 
>> On Tue, Aug 13, 2013 at 2:30 PM, Jim Grosbach <grosbach at apple.com> wrote:
>> Author: grosbach
>> Date: Tue Aug 13 16:30:58 2013
>> New Revision: 188315
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=188315&view=rev
>> Log:
>> DAG: Combine (and (setne X, 0), (setne X, -1)) -> (setuge (add X, 1), 2)
>> 
>> A common idiom is to use zero and all-ones as sentinal values and to
>> check for both in a single conditional ("x != 0 && x != (unsigned)-1").
>> That generates code, for i32, like:
>>   testl %edi, %edi
>>   setne %al
>>   cmpl  $-1, %edi
>>   setne %cl
>>   andb  %al, %cl
>> 
>> With this transform, we generate the simpler:
>>   incl  %edi
>>   cmpl  $1, %edi
>>   seta  %al
>> 
>> Similar improvements for other integer sizes and on other platforms. In
>> general, combining the two setcc instructions into one is better.
>> 
>> rdar://14689217
>> 
>> 
>> 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.
>> 
> 
> 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..
> 
> I'm not convinced it will actually show up from lowering in practice. 

I’m not convinced either way, really. Simply a mild motivation in one direction over the other. *shrug*

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?

> 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.
> 
> I don't follow.  Addition has the same semantics in IR and SelectionDAG.

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.

> 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.
> 
> Considering we already have code which catches "(x  !=  0) & (x != 1)", we should probably catch "(x != 0) & (x != -1)" there as well.

This makes a lot of sense, however.

Assuming we do add this to InstCombine, the question becomes whether to keep it in both places.

I’ll ponder a bit more.

-Jim
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130813/85a6e50e/attachment.html>