[llvm-commits] [PATCH] Teach IRBuilder about simplifying BinOp(Value, Constant)
Dan Gohman
gohman at apple.com
Sat Jan 3 11:12:27 PST 2009
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
More information about the llvm-commits
mailing list