[PATCH] D15302: [Greedy regalloc] Replace analyzeSiblingValues with something new [Part1]
Quentin Colombet via llvm-commits
llvm-commits at lists.llvm.org
Wed Mar 16 10:36:06 PDT 2016
> 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
>> 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
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.
More information about the llvm-commits