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

[Quentin Colombet via llvm-commits]

> On Jan 11, 2016, at 12:46 PM, Wei Mi <wmi at google.com> wrote:
> 
> wmi added a comment.
> 
>> #1
> 
>> 
> 
>> - Out of curiosity, do you have numbers of how many redundant spills we have for the current solution?
> 
>> - I am not saying the live-range splitting mechanism is perfect, but it fits nicely to the exiting framework, in particular w.r.t. the way we model the spill cost. Which leads me to #2.
> 
> 
> I did that experiment -- trying to catch redundent spills in current analyzeSiblingValues solution. I did catch some in llvm testsuite(~20, I don't remember exactly), but most of them are left there because of the HoistCondition checking in func propagateSiblingValue, .i.e, because of HoistCondition, current solution left some not very important but fully redundent spills in the code.

Thanks.
That means we have a gap of ~80 redundant spills with the split solution. Would be curious to see how many are actually important.

> 
>> #2
> 
>> 
> 
>> - hoistCopiesForSize is for Copies AFAIR, i.e., split points not spills. That means that, IIRC, we have the save cost model, since we do not care about split insertion in the cost.
> 
> 
> Could you elaberate what the save cost model mean here?

Sorry, typo, I meant same: regalloc and split kit are consistent regarding the cost model. In other words, improving the splitting will expose better spilling solutions for regalloc for free. 


> 
>> #3
> 
>> 
> 
>> - I agree the spill hoisting thing is more like a post reg alloc phase. Therefore, I would rather have it a post regalloc pass instead of embedded in the spiller. That being said, I understand this is easier to directly work with the virtual registers.
> 
>> 
> 
>>  To summarize, this is fine to have a the spill hoisting optimization where you put it. I believe though that making the splitting smarter would be the first logical step to mitigate the need for such optimization and I am still concerned that it may be bad for compile time.
> 
> 
> I compared the compile time between Jakob's solution (-disable-spill-analyze-sibvalue + -split-spill-mode=size) and my solution for some motivational testcases. They are the same. I will do more careful tests on this side, like using spec.

Thanks.

> 
>> If you’d like to pursue into that direction anyway, it is okay, just couple of inlined comments.
> 
>> 
> 
>> As for path part 2, it does not do anything at the moment does? (I.e., we clear the set before walking through it).
> 
> 
> The set DeadRemats is used to record def instructions which should have been removed when they are found to be dead after rematerialization. However, the def may still be useful for rematerialization of other siblings (Note without DefMI setting in analyzeSiblingValues, for all the siblings with equal values, the original_register_VNI->def is the best place to query the value expression. If the original def is deleted, we have no place to query value expression for rematerialization of siblings in the following rounds of selectOrSplit).

I am aware of that. What I was saying is that DeadRemats is not used to remove the instructions in the end (the patch clears the set before walking through it :)).

> So we decide to keep the dead instructions in their original places during the whole lifetime of allocatePhysRegs and use the set of DeadRemats to hold them (Changing the dest reg to a new dummy reg which will never be added to NewVRegs, so the live range can be updated properly in the same way as before). Those dead defs in DeadRemats are deleted after allocatePhysRegs is done.
> 
> 
> Repository:
>  rL LLVM
> 
> http://reviews.llvm.org/D15302
> 
> 
>