[Review]: Sink multiple instructions per iteration in InstCombine

Thu Jul 10 16:12:11 PDT 2014

Sorry it's taken me a while to get back to this.

I looked at the patch, and I'm still feeling like there has to be a more
direct way to use the instcombine worklist to address this problem rather
than adding another worklist on top. Having a feeling isn't a very good
code review though. =D So I grabbed your patch and tried to build a
benchmark just as you suggested to see what the performance difference was
and whether there was another way to achieve it.

However, despite building a benchmark that sinks some 400 instructions, I
can measure no performance improvement with your patch... I'm beginning to
suspect that my test case may just not be representative, could you attach
a (compressed if needed) test case that actually shows the performance
difference so that I can experiment with other approaches?

On Wed, Jul 2, 2014 at 10:49 AM, Aditya Nandakumar <
aditya_nandakumar at apple.com> wrote:

> Hi Chandler,
>
> Could you please look through the patch?
>
> Thanks
> Aditya
>
> On Jun 30, 2014, at 11:09 AM, Aditya Nandakumar <
> aditya_nandakumar at apple.com> wrote:
>
> Thanks Philip
>
> I have updated the patch with your suggested changes. Waiting for
> chandler’s review.
>
> Thanks
> Aditya
> <InstCombineUpdated.patch>
>
> On Jun 30, 2014, at 10:54 AM, Philip Reames <listmail at philipreames.com>
> wrote:
>
>
> 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>
> 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>
> 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> 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
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>>
>
>
>
> _______________________________________________
> llvm-commits mailing listllvm-commits at cs.uiuc.eduhttp://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/20140710/f6a51d65/attachment.html>