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

[Mikael Holmén via llvm-commits]

Hi Quentin and Wei,

On 04/28/2016 12:01 AM, Quentin Colombet wrote:
>
>> On Apr 27, 2016, at 2:53 PM, Wei Mi <wmi at google.com> wrote:
>>
>>>>
>>>> And does it matter that %vreg29 isn't marked as dead? If so, who should do that marking?
>>>
>>> The liveness analysis is supposed to do that IIRC. The dead flags are supposed to be properly set otherwise it is considered a bug.
>>>
>>> Cheers,
>>> -Quentin
>>
>> dead and kill flags are marked by LiveVariables analysis. However I
>> find the following paragraph of comment in Passes.cpp:
>>
>>   // FIXME: Once TwoAddressInstruction pass no longer uses kill flags,
>>   // LiveVariables can be removed completely, and LiveIntervals can be directly
>>   // computed. (We still either need to regenerate kill flags after regalloc, or
>>   // preferably fix the scavenger to not depend on them).
>>   addPass(&LiveVariablesID, false);
>>
>> That is why I don't see LiveVariables analysis required after phi
>> eliminatation pass. I guess following passes shouldn't depend on
>> dead/kill flags but depend on LiveIntervals analysis to get the
>> precise interval.
>>
>> %vreg29 at 528B becomes dead after RegisterCoalescer pass but its live
>> interval is not updated accordingly, and RegisterCoalescer declares to
>> preserve LiveIntervals. So may be it is RegisterCoalescer to be
>> blamed?
>
> Agreed, what I was saying was that I guess it is updated correctly if the kill/dead flags are set properly, which is what we usually test. So again, there are likely several bugs lurking in the regalloc related passes when used in that configuration.
> Given how obvious the problem seems to be, I wonder if it is safe to pretend that the coalescer knows how to work without the kill flags.
>
> Anyhow, it looks like the problem we got with Wei’s patch was a side effect of Mikael’s unorthodox configuration of the regalloc. Mikael, could you check if forcing the LiveVariables pass to run fixed the problem when Wei’s patch is in?

Ok, I tried to make LiveVariable to run again after RegisterCoalescer 
but hit a fatal error in LiveVariables since we're not on SSA anymore:

   if (!MRI->isSSA())
     report_fatal_error("regalloc=... not currently supported with -O0");

However, I managed to bring out the big hammer and "repair" LIS at the 
end of RegisterCoalescer, and then the code compiles succesfully!

So at the end of RegisterCoalescer I do

   // Make sure LIS is up to date
   LIS->repair(true);

Where "repair" and its help functions look like:

/// Clear all live intervals and rerun the analysis.
/// Relevant mailing list thread here:
/// http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-January/069187.html
void LiveIntervals::recompute(unsigned numVirtRegs) {
   DomTree->runOnMachineFunction(*MF);
   releaseMemory();
   VirtRegIntervals.resize(numVirtRegs);
   computeVirtRegs();
   computeRegMasks();
   computeLiveInRegUnits();

   addKillFlags(nullptr);
}

/// Moving and copying instructions may introduce disjoint live-ranges.
/// These need to be repaired by giving each disjoint live-range
/// its own virtual register.
void LiveIntervals::splitDisjointVRegs() {
   for (unsigned i = 0, e = MRI->getNumVirtRegs(); i != e; ++i) {
     unsigned Reg = TargetRegisterInfo::index2VirtReg(i);

     // Skip unused.
     if (MRI->reg_nodbg_empty(Reg))
       continue;

     LiveInterval &LI = getInterval(Reg);
     assert(Reg == LI.reg && "Invalid reg to interval mapping");

     if (!TargetRegisterInfo::isVirtualRegister(LI.reg))
       continue;

     // Check whether or not LI is composed by multiple connected
     // components and if that is the case, fix that.
     SmallVector<LiveInterval*, 8> SplitLIs;
     splitSeparateComponents(LI, SplitLIs);
   }
}

/// Clear all live intervals and rerun the analysis.
/// If ordered, split disjoint live-ranges and give each live-range
/// its own new virtual register
void LiveIntervals::repair(bool SplitLiveRanges) {
   assert(MRI);

   const unsigned NumVRegs = MRI->getNumVirtRegs();
   recompute(NumVRegs);

   if (!SplitLiveRanges)
     return;

   splitDisjointVRegs();

   // If new vregs have been added, recompute again.
   if (NumVRegs != MRI->getNumVirtRegs())
     recompute(MRI->getNumVirtRegs());
}

This repair function is something we've written earlier to be able to 
fixup LIS after we've changed the code so LIS is invalid (and it's been 
cumbersome for us to keep it updated "on the fly", and we've been on 
non-SSA form) so now I tried using it after the coalescer as well.

With this, the dead def at 528 (which wasn't marked as dead) now is 
split into its own LI, with its own vreg, and now the def is indeed 
marked as dead:

528B		%vreg30<def,dead> = mv_ar16_ar16_lo16In32 %vreg13<kill>, pred:0, 
pred:%noreg, pred:0, %ac0<imp-use>, %ac1<imp-use>; aN32_0_7:%vreg30 
aNh_0_7:%vreg13

And the live intervals of both vreg29 and vreg30 looks valid.

I haven't been able to test this very thouroughly yet, but so far so good.

However, I'm still curious... do you say that RegisterCoalescer should 
work and keep LiveVariables properly updated even without 
kill/dead-flags? Or is already the input to the coalescer broken since 
kill flags are missing?

Is it a bug that LiveVariables removes the kill-flags when it's run 
somewhere prior to RegisterCoalescer or is this ok?

So when kill and dead flags should be present and who should update them 
is still quite unclear for me.

And if the register allocator has assumptions about how the input code 
should look, shouldn't there be some checks that those assumptions are 
fulfilled rather than ending up with invalid live ranges after regalloc 
that the verifier shouts about?

Thanks alot for your help,
Mikael

>
> Thanks,
> -Quentin
>
>>
>> Thanks,
>> Wei.
>