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

Quentin Colombet via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Jul 21 18:09:46 PDT 2017


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) &&
----------------
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


https://reviews.llvm.org/D35730





More information about the llvm-commits mailing list