[Review]: Sink multiple instructions per iteration in InstCombine

[Philip Reames]

Just to make sure I understand the issue properly, the key change here 
is adding the *operands* rather than *users* to *a* worklist right?  We 
could potentially add them to the outer worklist and get the same benefit?

Looking through the code, we only seem to add the users of a particular 
instruction to the worklist.  For the sinking code, it does make sense 
to reprocess the operands since sinking the original instruction may 
have opened up further opportunities.

As a side note, I do find myself somewhat wondering why we mix 
transforms and placement here.  The addition of the sinking code stands 
out in this loop.  Anyone know why this is done here rather than another 
pass?  Just curious about the history.

Philip

On 06/27/2014 01:17 PM, Aditya Nandakumar wrote:
> Thanks Chandler for your review.
> The problem I saw in a few test cases was that instcombine had 
> finished visiting all instructions (Iteration#0) and the only possible 
> changes were sinking of instructions. This sinking of one instruction 
> opened up sinking opportunities for other instructions which were only 
> being used by the current(sunk) instruction. Since we are visiting 
> top-down, there is only one instruction being sunk per iteration.
> So in some my test cases, instcombine ran for 8 iterations where in 
> iterations 1-8, it sank one instruction per iteration. The test cases 
> are about 150-300 lines long and we are visiting all those 
> instructions every iteration even though the only change possible is 
> the sinking.
>
> Consider the following example.
> bb1: %d0 = ...
> %d1 = .. %d0..
> %d2 = .. %d1..
> %d3 = op %d2 ..
> ...
> bb2:
>       .. =  op %d3
>
> Currently instcombine would sink d3 in Iteration#0, d2 in Iteration#1, 
> d1 in Iteration#2 and d0 in Iteration#3 - but it only requires one 
> iteration in all.
>
> Updated patch - removed C style comments and fixed typo(I think).
>
>
>
>> On Jun 26, 2014, at 5:54 PM, Chandler Carruth <chandlerc at google.com 
>> <mailto:chandlerc at google.com>> wrote:
>>
>> I don't immediately understand why this is such a performance 
>> improvement here (and not elsewhere in instcombine)... Do you have 
>> any insight into that? Is it a locality problem that we don't 
>> re-visit the other sinkable instructions in the right order? It feels 
>> pretty weird to have a worklist-inside-a-worklist, so I'm just trynig 
>> to understand it better...
>>
>> Also, the patch has a bunch of typos in comments and uses C-style 
>> comments...
>>
>>
>> On Mon, Jun 23, 2014 at 10:57 PM, Aditya Nandakumar 
>> <aditya_nandakumar at apple.com <mailto:aditya_nandakumar at apple.com>> wrote:
>>
>>     Hi All
>>
>>     I want to make a small optimization for instcombine. While
>>     sinking instructions (with single use) to the use block, also
>>     check if any other instructions which are used in the current
>>     instruction (operands) can be sunk.
>>
>>     This speeds up InstCombine time in several cases as previously
>>     only one instruction would be sunk per iteration.
>>
>>     Please review
>>
>>     Thanks
>>     Aditya
>>
>>     _______________________________________________
>>     llvm-commits mailing list
>>     llvm-commits at cs.uiuc.edu <mailto:llvm-commits at cs.uiuc.edu>
>>     http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>>
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140627/2bf84761/attachment.html>