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

[Török Edwin]

On 2009-01-03 21:12, Dan Gohman wrote:
> Hi Török,
>
> This patch looks very useful. I have a few comments, below.
>   

Hi Dan,

I've made the changes according to your comments, see the new patch
attached.
For convenience I also attached an 'interdiff -w simplifier2.patch
simplifier3.patch'.

To summarize:
- renamed my version of ConstantFoldInstruction to SimplifyInstruction
[I've kept the old ConstantFoldInstruction with Constant* return value
intact]
- remove my version of ConstantFoldInstOperands, because it was just
leading to duplicated code:
    if we want to constant fold/simplify before creating, we can use
IRBuilder already
- removed the dead include
- limit iteration on uses to max 8
- check that the use we found is actually in the same BB (was missing a
lower-bound check)
- reindent case inside switch
-  use ConstantFolder.h instead of TargetFolder.h in ConstantFolding.cpp
- make SimplifyInstruction behave like SimplifyInstOperands
- make SimplifyInst do the select optimization that IRBuilder was
already doing in v2

See below for details.

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

Done.

> A few other misc. comments:
>
> Is TargetFolder.h really needed in ConstantFolding.cpp?
>   

I don't want to duplicate code, so I want to reuse what I already
implemented in the folders.
Though Support/ConstantFolder.h should be enough, so I changed
TargetFolder.h to ConstantFolder.h now.

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

Oops, that was an oversight. That file isn't actually needed anymore in
v2 of my patch, I'll remove the #include.

>  > +      switch (Opc) {
>  > +        case Instruction::Add:
>
> LLVM code typically does not have extra indentation for case statements.
>   

How do I tell my VIM's autoindent to comply? I tried using
utils/vim/vimrc, but it puts the case to the same place.
(For now I manually indented the first case, and reindented the rest).

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

Ok.

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

You are right. I won't iterate on all uses, just on maximum 8 (for
example) uses.
This way I will still catch the trivial cases, and not slow down on
large BBs / values with lots of uses.
I'll leave it up to an optimization pass that is run later to handle the
non-trivial cases (I think GVN already does this).

Best regards,
--Edwin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: simplifier3.patch
Type: text/x-diff
Size: 28750 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20090103/f2857cab/attachment.patch>
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: interdiff.txt
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20090103/f2857cab/attachment.txt>