[Review]: Sink multiple instructions per iteration in InstCombine

[Philip Reames]

On 06/30/2014 10:45 AM, Aditya Nandakumar wrote:
> Yes - the key change is to add the operands.
> You are also right in that we could just add the operands to the outer 
> work list and get the same benefit. I was also wondering why the 
> sinking code is in InstCombine.
> I have updated the patch to just add to the outer work list.
Thanks.  The revised change is much more clear.

With the change mentioned below, this LGTM.  You should wait for 
Chandler's signoff as well.

Minor comments:
- It looks like you can use the for-range loop here.  for( auto op : 
I->operands() ) {...}
- Update your comment to emphasis the operands part.  This was what both 
I and Chandler didn't see at first in your original change set.  
Something along the lines of "We'll add uses of the sunk instruction 
below, but since sinking can expose opportunities for it's *operands* 
add them to the worklist."
- I'd structure the conditional as:
if( TryToSinkInstruction(..) ) { MadeIRChange = true; for... };

Philip

>
>
>
>
> On Jun 27, 2014, at 4:23 PM, Philip Reames <listmail at philipreames.com 
> <mailto:listmail at philipreames.com>> wrote:
>
>> 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/20140630/aa457660/attachment.html>