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

Tue Apr 26 11:16:08 PDT 2016

On Tue, Apr 26, 2016 at 11:01 AM, Wei Mi <wmi at google.com> wrote:
> +Quentin and llvm-commits.
>
> On Tue, Apr 26, 2016 at 1:05 AM, Mikael Holmén
> <mikael.holmen at ericsson.com> wrote:
>> Hi Wei,
>>
>> On 04/26/2016 02:24 AM, Wei Mi wrote:
>>>
>>> Hi Mikael, after some discussions with Quentin
>>>
>>> (http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20160425/350786.html),
>>> I think it is necessary for me to look at your testcases more closely,
>>> because in theory the errors shouldn't happen. I must be missing
>>> something.
>>>
>>> Is it possible to send out preprocessed files and the command lines to
>>> reproduce the problems you saw?
>>
>>
>> Well, this happens only when compiling for our out-of-tree target, so
>> unfortunately we can't do that. We need to have our hardware's set of
>> registers and intructions etc for the register allocator to behave exactly
>> the way it does for us. Maybe it's possible to reproduce this on some other
>> target, but from experience it's quite hard to do that with register
>> allocator problems. :/
>>
>> So I've compiled a bugpoint-reduced version of the code again now, with and
>> without your latest patch "Keep dead inst copy from being deleted only when
>> the inst is rematerializable", with debug printouts on.
>>
>> I added one printout in LiveRangeEdit::checkRematerializable
>>
>> +  DEBUG(dbgs() << "LiveRangeEdit::checkRematerializable: Checking
>> isTriviallyReMaterializable on: " << *DefMI);
>>    if (!TII.isTriviallyReMaterializable(DefMI, aa)) {
>> +     DEBUG(dbgs() << "false\n");
>>      return false;
>>    }
>> +   DEBUG(dbgs() << "true\n");
>>
>> and one in LiveRangeEdit::eliminateDeadDef just before your change
>>
>> +    DEBUG(dbgs() << "LiveRangeEdit::eliminateDeadDef: Checking
>> isTriviallyReMaterializable on: " << *MI);
>> +    if (TII.isTriviallyReMaterializable(MI, AA))
>> +      DEBUG(dbgs() << "true\n");
>> +    else
>> +      DEBUG(dbgs() << "false\n");
>> +
>>     if (isOrigDef && DeadRemats && TII.isTriviallyReMaterializable(MI, AA))
>> {
>>        LiveInterval &NewLI = createEmptyIntervalFrom(Dest);
>>        VNInfo *VNI = NewLI.getNextValue(Idx, LIS.getVNInfoAllocator());
>>
>> I attached the logs from the two runs.
>>
>> Note that with your patch the code compiles succesfully now, so it doesn't
>> seem to be totally rubbish :)
>>
>> Of course the logs now have some stuff from our backend, some instructions
>> and registers you don't know, but maybe you can figure out what's going on
>> anyway. If I can add some more printouts that you want to be able to
>> understand better what's happening, please don't hesitate to tell me and
>> I'll add and rerun.
>>
>> Thanks for looking into this,
>> Mikael
>>
>
> Hi Mikael,
>
> Thank you very much for providing a crash log. It is very helpful. I
> look at foo.crash.log and find a speciality in foo.red.ll:
> The def to vreg29 at 528r is in an infinite loop. Although the def
> actually has no use, it is not marked as dead and these segments:
> [336B,448B:0)[528r,608B:1)[608B,720B:2) exists only because the dead
> def at 528r.
>
> Before greedy regalloc starts:
> %vreg29 [48r,336B:3)[336B,448B:0)[528r,608B:1)[608B,720B:2)[720B,976B:3)
>  0 at 336B-phi 1 at 528r 2 at 608B-phi 3 at 48r
>
> next step:
> %vreg29 is splitted into %vreg31 and %vreg32.
>
> next step:
> 752B:2 libcall_CRT_ll_div_r %vreg19, %vreg19, %vreg32, %vreg1,
> %a0_32<imp-def>, %a1_32<imp-def>, %CCReg<imp-def,dead>, ...;
> aN32_0_7:%vreg19,%vreg32,%vreg1
> is rematerialized, and %vreg32 at 312r becomes dead.
>
> next step:
> When it deletes 312r %vreg32<def,dead> = COPY %vreg31;
> aN32_0_7:%vreg32,%vreg31, it shrink the live range of vreg31.
> ---------- trace extracted from foo.crash.log -----------
> Shrink: %vreg31 [48r,312r:0)[336B,448B:1)[528r,608B:2)[608B,720B:3)
> 0 at 48r 1 at 336B-phi 2 at 528r 3 at 608B-phi
>  live-in at 176B
> Dead PHI at 336B may separate interval
> All defs dead: 528r %vreg31<def,dead> = mv_ar16_ar16_lo16In32 %vreg13,
> pred:0, pred:%noreg, pred:0, %ac0<imp-use>, %ac1<imp-use>;
> aN32_0_7:%vreg31 aNh_0_7:%vreg13
> Dead PHI at 608B may separate interval
> Shrunk: %vreg31 [48r,128B:0)[176B,192r:0)[528r,528d:2)  0 at 48r 1 at x 2 at 528r 3 at x
>   Split 2 components: %vreg31 [48r,128B:0)[176B,192r:0)[528r,528d:1)
> 0 at 48r 1 at 528r
> Deleting dead def 528r %vreg34<def,dead> = mv_ar16_ar16_lo16In32
> %vreg13, pred:0, pred:%noreg, pred:0, %ac0<imp-use>, %ac1<imp-use>;
> aN32_0_7:%vreg34
> ---------------------------------------------------------------------
> Note that %vreg34 is generated from %vreg31. When 312r is deleted, it
> shrinks the live range and finds 528r is dead at the same time. !!!
> But the def of 528r has no relationship with the use of %vreg31 at
> 312r !!!
>
> 528r %vreg34<def,dead> = mv_ar16_ar16_lo16In32 %vreg13  is not
> triviallyRematerializable, but because it is an OriginalDef, it can
> satisfy the condition to go into DeadRemat. This is how we get a case
> that an instruction in DeadRemat is not triviallyRematerializable.
>
> Thanks,
> Wei.

Hi Quentin,

>From the analysis of crash report given by Mikael, the error happens
in a very rare case -- dead live interval and dead VNI are created
before regalloc, and then stay there until live range shrinking is
called because of remat for other unrelated uses. In such case,
non-triviallyRematerializable instructions can slip into DeadRemat
set.

I think adding triviallyRematerializable check can guard the safety
for this rare case for sure, but I don't know if we have a better way
to remove the weird case earlier -- dead live intervals and dead VNI
should be removed before regalloc.

Thanks,
Wei.