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

[Török Edwin]

On 2009-01-04 05:26, Dan Gohman wrote:
> I fixed utils/vim/vimrc; it should now do the right thing.
>   

Thanks.
Since you know more about vim indenting than me, here are a couple more
things that I have to do manually, to make my code look like
the rest in IRBuilder.h (I assume these are part of the LLVM coding
style too?):
- class is not indented vs namespace (vim wants to indent)
- public: is not indented vs class (vim wants to indent)
- when I wrap arguments due to exceeding 80-col, vim doesn't align the
beginning with the ( on previous line, so I have to manually tab/space
align it

Are these fixable?

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

Ok, I changed reuseOps to false there.

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

Indeed, fixed.

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

Oops. In that case I only check that the instruction is in the same
BasicBlock, and require the caller
to not move the insertion point backwards. This should ensure that the
Value I return exists at the current insertionpoint.
[I could iterate and check that the instruction is really there, but
that would be a waste of time, since I know it is...]
Since this is a default off feature (requiring an explicit template
parameter to activate) I think it is ok.
What do you think?

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