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

[Chris Lattner]

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.

> 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.

>
> * 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?

> 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.

> 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.

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!

-Chris