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

[Wei Mi via llvm-commits]

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.

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?

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

Regards,
Wei.