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

[Dan Gohman]

Hi Török,

This patch looks very useful. I have a few comments, below.

On Dec 30, 2008, at 8:08 AM, Török Edwin wrote:

> On 2008-12-29 19:43, Török Edwin wrote:
>> Will submit a new patch later.
>>
>
> Here is the new patch, noteworthy changes:
> - simplifications implemented in ConstantFolder (default on)
> - except for the find-identical-instruction simplification, which is
> default off, configurable via template parameter
> - for binops there is now only CreateBinOp in constantfolder/nofolder
> - NoFolder now simply returns 0 for binops, and lets IRBuilder create
> the binary operator, and insert it
> (I wonder how it ever worked before, without inserting the instruction
> it created?)
> - TargetFolder is derived from ConstantFolder now, to reuse the
> simplifications
> - new variants of ConstantFoldInstruction/ConstantFoldInstOperands  
> that
> do these simplifications also

Perhaps ConstantFoldInstruction should be named SimplifyInstruction? We
may want to extend it to things like x-x, x&x, x|x, etc., which  
technically aren't
constant folding.

A few other misc. comments:

Is TargetFolder.h really needed in ConstantFolding.cpp?

It appears NoSimplifier.h is missing in the patch.  Also, does  
IRBuilder.h really
need to include it?

 > +      switch (Opc) {
 > +        case Instruction::Add:

LLVM code typically does not have extra indentation for case statements.

 > +  inline Value *CreateNeg(Value *V, bool reuseOps,
 > +                     const BasicBlock::iterator *I) const {

The inline keyword here is redundant, since this is a member function
defined inside a class.  It isn't an official style rule, but we're  
gradually
moving toward being more consistent about this.

 > +    // There should be less uses than instructions in a BB, so  
this should be
 > +    // faster than iterating over all instructions in BB.

There may be fewer uses than instructions within one block, however the
use list includes uses from all blocks, which is a potential  
efficiency concern
here.

Dan