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

[Dan Gohman]

On Jan 3, 2009, at 1:30 PM, Török Edwin wrote:

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

Thanks!

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

I fixed utils/vim/vimrc; it should now do the right thing.

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

Ok.  It's interesting that the first 8 uses that will be searched will
be the most recently created uses, due to the way the use list
currently works.  That's pretty handy for front-ends, though we
should be careful not to depend on this too much.

A few more misc. comments:

I don't think SimplifyInstruction should set reuseOps to true.  When
SimplifyBinOp is called from a front-end creating new IR, it can be
useful to avoid creating new redundant instructions. But
SimplifyInstruction operates on existing IR, and if the IR already
has redundant instructions, it's not as beneficial for it to do extra
work to eliminate them here.  Also, the use lists are less likely
to be in an adventageous order.

SimplifyInstruction doesn't need to check that the operands are
constant before calling ConstantFoldInstruction. It already checks
that, and returns null if it didn't fold.

This code in findBinOp isn't right:

             (P <= *I && P > UI->getParent()->begin()))

BasicBlock::iterator is not random-access so it doesn't support <= or >.
Unfortunately, what's probably happening here is both sides being
implicitly converted to pointers which are then compared, which is
not good.  This is the dark side of LLVM's style of iterators :-(.  I  
just
added some code which I believe will catch this error at compile-time
now.

Dan