[llvm-commits] [PATCH] Teach IRBuilder about simplifying BinOp(Value, Constant)

[Török Edwin]

On 2008-12-29 19:30, Chris Lattner wrote:
> On Dec 28, 2008, at 12:07 PM, Török Edwin wrote:
>
>   
>> Hi,
>>
>> I have a pass that produces lots of and,or,xor where one of the  
>> operands
>> is a constant (0 or all ones).
>> Initially I thought to run instcombine afterwards to clean up, but  
>> now I
>> think  it is more efficient to not create those instructions in the
>> first place:
>> why create possibly hundreds of instructions just to delete them  
>> later.
>>     
>
> Sure, this makes sense for simple things.  I'm fine with slightly  
> "feature creeping" IRBuilder to support more simple foldings.  I also  
> agree with Duncan's guidance downthread.
>   

Ok.

>   
>> There is also a BinOpSimplifier class in my patch that handles some  
>> very
>> simple (and common for me) cases:
>> * I have only implemented very simple cases, the more cases belong to
>> instcombine (and AFAICT they are there already)
>>     
>
> RIght.
>
>   
>> * if instruction already exists in BB before or at insertion point,
>> reuse it
>>    If BinOpSimplifier ever becomes the default for IRBuilder, this
>> could cause problems if somebody inserts an instruction using  
>> IRBuilder,
>>       and later uses RAUW if !isa<Constant>, because it can  
>> potentially
>> replace more than he thought of!
>>     
>
> This is interesting.  As long as it doesn't scan an unbounded number  
> of instructions, I'm ok with it.
>   

Yep its bounded, it only scans uses of LHS/RHS, which should be less
than the number of instructions in a BB.

>   
>> * if the outcome of a binop is known to be a constant, return the  
>> constant:
>>       and X, 0 -> 0; and X, ~0 -> X; or X, 0 -> X; or X, ~0; xor X,  
>> 0 -> X;
>> * simplify chained XORs with same constant,:
>>    If we want to create xor %1, C; and there is a "%1 = xor %0,  C",
>> then return %0
>>     
>
> Why this one in particular?  What cases does this happen with?   
> IRBuilder is meant largely to be a utility for frontend developers.  A  
> lot of this sort of stuff should be driven by real use cases in the  
> front-end.  Is this simplification really *important* or are you just  
> adding it because it is easy?
>   

I don't think that my pass will produce this kind of chained xors, but:
- its straight forward (and fast?) to implement
- I've seen lot of LIKELY/UNLIKELY macros implemented as
__builtin_expect(!!(x), 0/1) (Linux kernel, Xorg, ...)

I thought that to be a good enough reason to implement it ;)

>   
>> While we're here I also implemented:
>> * add X, 0 -> X
>> * sub X, 0 -> X
>> * mul X, 0 -> 0
>> * mul X, 1 -> X
>> * udiv/sdiv 0, X -> 0
>> * udiv/sdiv X, 1 -> X
>> * urem X, 1 -> 0
>>     
>
> I'm fine with these simple local xforms.  Please verify that  
> instcombine does them all.
>   

Will do.

>   
>> I haven't implemented shifts, because I am not sure how complicated  
>> we'd
>> want to make this: use ComputeMaskedBits, and make decision based on
>> that, or
>> just simplify shift by 0, and shift > bitwidth?
>>     
>
> Do not use ComputeMaskedBits.  Only use local information, such as a  
> zero shift amount.  The point of doing this sort of thing is doing  
> really trivial simplifications of the same sort of things as  
> SelectionDAG::getNode does, we do not want "yet another" instcombine.
>   

Indeed.

> In addition to Duncan's comments, please try to follow the LLVM style  
> more closely: open braces on the previous line, "reason strings" in  
> assertions, comments before loops, etc.
>
> Thanks Edwin!
>   

Will submit a new patch later.

Thanks for the comments,
--Edwin