[PATCH] D35730: RA: Remove assert on empty live intervals

[Quentin Colombet via llvm-commits]

> On Jul 21, 2017, at 6:39 PM, Quentin Colombet via llvm-commits <llvm-commits at lists.llvm.org> wrote:
> 
>> 
>> On Jul 21, 2017, at 6:37 PM, Quentin Colombet via Phabricator <reviews at reviews.llvm.org> wrote:
>> 
>> qcolombet added inline comments.
>> 
>> 
>> ================
>> Comment at: lib/CodeGen/RegAllocBase.cpp:147
>>      DEBUG(dbgs() << "queuing new interval: " << *SplitVirtReg << "\n");
>> -      assert(!SplitVirtReg->empty() && "expecting non-empty interval");
>>      assert(TargetRegisterInfo::isVirtualRegister(SplitVirtReg->reg) &&
>> ----------------
>> MatzeB wrote:
>>> qcolombet wrote:
>>>> qcolombet wrote:
>>>>> MatzeB wrote:
>>>>>> qcolombet wrote:
>>>>>>> MatzeB wrote:
>>>>>>>> MatzeB wrote:
>>>>>>>>> arsenm wrote:
>>>>>>>>>> qcolombet wrote:
>>>>>>>>>>> qcolombet wrote:
>>>>>>>>>>>> arsenm wrote:
>>>>>>>>>>>>> qcolombet wrote:
>>>>>>>>>>>>>> If that's empty and legit, I believe we shouldn't try to enqueue it.
>>>>>>>>>>>>> It still needs to be allocated to a register, otherwise an undef vreg use is left around
>>>>>>>>>>>> The problem IIRC is that I believe later code is not fine with that.
>>>>>>>>>>>> (BTW, you could have waited for the discussion to be over before committing...)
>>>>>>>>>>> Regardless of the surrounding code working or not in that case, having an empty live-range that needs a register sounds plain wrong to me.
>>>>>>>>>> Sorry, I had talked to Matthias about this earlier. The assert is also fairly new and was added last year in r279625 with subregister changes.
>>>>>>>>> An empty live range that needs a register looks like this and is valid IMO and the normal code handled it fine in my experiments (but it needs to assign a register obviously):
>>>>>>>>> ```
>>>>>>>>> = USE vreg0<undef>
>>>>>>>>> ```
>>>>>>>>> 
>>>>>>>> Ah the infamous r279625, let's add Krzysztof to the reviwers.
>>>>>>> I am fine with removing the assert. I'm against enqueueing an empty live-range though. More specifically, I am fine with enqueueing it as long as it is valid not to assign it a register.
>>>>>>> We have code that makes that assumption (e.g., in last chance recoloring).
>>>>>> We need to enqueu it so it gets a register assigned. I think those empty liveranges will always succedd when we try to assign a register (because they interfer with nothing) so we should never hit the recolring, splitting etc. phases for them.
>>>>> ```
>>>>> = USE vreg0<undef>
>>>>> ```
>>>>> That's different.
>>>>> That says we don't care about the value of vreg0 here, not that its live-range is empty.
>>>>> 
>>>>> What I am saying is that I would rather that we come up with a different concept than overloading empty here, because it weakens our ability to find bugs.
>>>>> We need to enqueu it so it gets a register assigned.
>>>> 
>>>> Yes because how we conflates the semantic of empty and need a register. But I don't think that's right.
>>>> 
>>>>> [...] so we should never hit the recolring[...]
>>>> 
>>>> Although that's probably true, each time I see should, I think potential bugs :P
>>> undef operand by construction have no influence on the liverange. They are perfectly valid operands that use vregs, but they have absolutely no effect on the liverange. Hence we end up with an empty liverange. That is just how things work AFAIK.
>>> undef operand by construction have no influence on the liverange. They are perfectly valid operands that use vregs, but they have absolutely no effect on the liverange. Hence we end up with an empty liverange.
>>> That is just how things work AFAIK.
>> 
>> I agree that empty live-range model the right thing.
>> What I am saying is that I want to distinguish between something that does not affect liveness because we don't care about its value (this we should enqueue) and something that does not affect liveness but should be here (e.g., a bug that produced and invalid live-range).
> 
> *shouldn't
> 
>> 
>> To be concrete, I think we want to do instead:
>> assert((!LI->empty() || !MRI.reg_nodbg_empty()) && "Empty and not used live-range?!”)

Note: Matthias pointed out that this is tautological with the surrounding code. This is more a guidance of how we should upgrade those asserts instead of removing them.

>> 
>> 
>> https://reviews.llvm.org/D35730 <https://reviews.llvm.org/D35730>
>> 
>> 
>> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits <http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170721/16e0b5d0/attachment.html>