[PATCH] D15302: [Greedy regalloc] Replace analyzeSiblingValues with something new [Part1]

[Quentin Colombet via llvm-commits]

Hi Wei,

> On Mar 16, 2016, at 10:06 AM, Wei Mi <wmi at google.com> wrote:
> 
> On Tue, Mar 15, 2016 at 5:37 PM, Quentin Colombet <qcolombet at apple.com> wrote:
>> qcolombet requested changes to this revision.
>> qcolombet added a comment.
>> This revision now requires changes to proceed.
>> 
>> Hi Wei,
>> 
>> The benchmarks are still running, but so far, the numbers look good.
>> 
>> Anyhow, I finally made time for the review.
>> 
> 
> Quentin, many thanks for the testing and detailed review.
> 
>> Generally speaking this looks almost good to me. The quadratic behavior of the first loop in runHoistAllSpills scares me and we need to look for a better alternative.
> 
> It is easy to fix. I can simply remove the inner loop. It will
> increase the size of SlotToOrigReg map to be the same as that of the
> virtRegs set being allocated to stack, so the increase shouldn't be
> much.
> 
>> Moreover we need better comments for the APIs.
> 
> I will fix it according to your inline comments.
> 
>> Finally, the tests change with more moves are worrisome. Could you explain why this happens and how we will fix that?
> 
> This is because after we remove analyzeSiblingValues, we also remove
> the logic to move spills to an earlier position in the same BB which
> can relieve reg pressure in some cases -- there is a paragraph of
> comment describing the cases in InlineSpiller::propagateSiblingValue
> (Search "This is an alternative def earlier in the same MBB" to find
> it). And this logic should be done during allocatePhysRegs(), not
> after that, so I cannot add it in HoistSpiller::hoistAllSpills. I
> havn't thought it carefully, but probably I will place the logic
> somewhere inside or close to SplitEditor::hoistCopies.

That would make sense to me.

> 
> This is the task I plan to work on after this patch, and because it is
> a BB local optimization, it should be an easy task. Another way is to
> work on the local hoisting task first, and combine the patches to
> avoid changing the tests back and force. Which way you think is
> better?

Good I wanted to make sure you were planning to work on that.

Avoiding to change the tests back and forth is probably better, but it is up to you :).

> 
>> It seems to me we are choosing an insertion point for the store that happens too late.
>> 
>> I have highlighted all my concerns with the inline comments.
>> 
> 
> I am working on it.

Thanks,
-Quentin
> 
> Regards,
> Wei.