Another problem with "Recommit r265547, and r265610, r265639, r265657"

[Wei Mi via llvm-commits]

Hi Mikael,

Thanks for the analysis!

> So some shrinking of the live range is needed in this case. I saw that there
> is some code in the coalescer to do this:
>
>   if (ShrinkMainRange) {
>     LiveInterval &LI = LIS->getInterval(CP.getDstReg());
>     shrinkToUses(&LI);
>   }
>
> but ShrinkMainRange is false all the time so that code isn't run. However,
> if I force ShrinkMainRange to be true I see:
>
> Shrink: %vreg29 [144r,176B:3)[336B,448B:0)[576r,608B:1)[608B,720B:2)
> 0 at 336B-phi 1 at 576r 2 at 608B-phi 3 at 144r
> Dead PHI at 336B may separate interval
> Dead PHI at 608B may separate interval
> Shrunk: %vreg29 [144r,144d:3)[576r,576d:1)  0 at x 1 at 576r 2 at x 3 at 144r
>   Split 2 components: %vreg29 [144r,144d:3)[576r,576d:1)  0 at x 1 at 576r 2 at x
> 3 at 144r
>         Success: %vreg7 -> %vreg29
>         Result = %vreg29 [144r,144d:2)  0 at x 1 at x 2 at 144r
>
> so the range for %vreg29 is shrunk, and also the two (now dead) defs are
> split into %vreg29 and %vreg30.
>
> I'm a little concerned about the "0 at x 1 at x" things in %vreg29's range which
> I've no idea if they do any harm and if or how they should be removed.

I think they are fine. @x means the VNI is unused. Look at function
LiveRange::markValNoForDeletion. It is a lazy way to delete VNIs from
valnos.

> which looks reasonable to me. The whole progam now compiles succesfully even
> if I turn off the repair thing I added previously.
>
> So, we just need to figure out in what cases we need to do the shrinking,
> when to set ShrinkMainRange to true. Currently it's only done in
> RegisterCoalescer::addUndefFlag called from
> RegisterCoalescer::updateRegDefsUses so something more is needed.
>
> I suppose the reason for only doing the shrinking sometimes is to save
> compilation time? It should always be ok to run shrinkToUses (on virtual
> registers), but in most cases it's not necessary so that's why it's avoided?
>
> So in my code now I do
>
>   // Somewhat brute solution to #10204. Always shrink live ranges for
> virtual
>   // registers. Ideally we could identify the exact cases where it's needed
>   // but for now we shrink the range for all vregs.
>   if (ShrinkMainRange ||
>       TargetRegisterInfo::isVirtualRegister(CP.getDstReg())) {
>     LiveInterval &LI = LIS->getInterval(CP.getDstReg());
>     shrinkToUses(&LI);
>   }
>
> and then it seems to work.
>

The analysis is reasonable and I cannot come up a solution better than
yours :-). Although ideally we can have a solution in the end which
don't penalize normal cases.

Thanks,
Wei.