[llvm] r265317 - Revert r265309 and r265312 because they caused some errors I need to investigate.

Fri Apr 22 12:38:15 PDT 2016

Hi Quentin,

First I want to thank you for so much help to review and improve the
patches. I didn't receive bug reports for some days and I thought
maybe I can tell you it is good to reapply the r263460,  however
yesterday Mikael Holmén sent another two error reports to me. The
first one is easy to fix and I have tested the patch. I am still
thinking the best way to fix the other one. Here are the reports.

Thanks,
Wei.

==================================================================================
The first report:
==================================================================================
After even more testing we've run into a new problem related to your commit
 "Recommit r265547, and r265610,r265639,r265657 on top of it, plus"

In LiveRangeEdit.cpp:

    if (isOrigDef) {
      LiveInterval &NewLI = createEmptyIntervalFrom(Dest);
      VNInfo *VNI = NewLI.getNextValue(Idx, LIS.getVNInfoAllocator());
      NewLI.addSegment(LiveInterval::Segment(Idx, Idx.getDeadSlot(), VNI));
      pop_back();
      markDeadRemat(MI);
      const TargetRegisterInfo &TRI = *MRI.getTargetRegisterInfo();
      MI->substituteRegister(Dest, NewLI.reg, 0, TRI);
      MI->getOperand(0).setIsDead(true);
    } else {

What if the destination of MI is tied to one of the input operands? Then

 MI->substituteRegister(Dest, NewLI.reg, 0, TRI);

will change both the def and the use to be the new vreg, and suddenly
we get a use of an undefined register. That seems to happen for me in
my out-of-tree target in some cases now.

I turned on -debug printouts and I see:

All defs dead: %vreg65<def,dead,tied1> =
addh_nimm5_a32_aNhDst_formatDstN32 %vreg65<tied0>, 4
Deleting dead def 1212r %vreg65<def,dead,tied1> =
addh_nimm5_a32_aNhDst_formatDstN32 %vreg65<tied0>, 4

When I stop at the "if" above we have:

176B    BB#1: derived from LLVM BB %vector.body
[...]
192B            %vreg65<def,tied1> =
addh_nimm5_a32_aNhDst_formatDstN32 %vreg65<tied0>, 4
212B            %vreg60<def> = COPY %vreg65
[...]
240B            bkrepLabelPseudo <BB#1>, 0
            Successors according to CFG: BB#2 BB#1
1116B   BB#2: derived from LLVM BB %bb3
[...]
1212B           %vreg65<def,dead,tied1> =
addh_nimm5_a32_aNhDst_formatDstN32 %vreg65<tied0>, 4

MI is:
  %vreg65<def,dead,tied1> = addh_nimm5_a32_aNhDst_formatDstN32 %vreg65<tied0>, 4

And then after substituteRegister we get:

  %vreg67<def,dead,tied1> = addh_nimm5_a32_aNhDst_formatDstN32 %vreg67<tied0>, 4

And then the verifier gets upset with me:

*** Bad machine code: No live segment at use ***
- function:    f1
- basic block: BB#2 bb3 (0x2248698) [1116B;1292B)
- instruction: 1212B    %vreg67<def,dead,tied1> =
addh_nimm5_a32_aNhDst_formatDstN32
- operand 1:   %vreg67<tied0>
- liverange:   [1212r,1212d:0)  0 at 1212r
- v. register: %vreg67
- at:          1212B

*** Bad machine code: Virtual register def doesn't dominate all uses. ***
- function:    f1
- basic block: BB#2 bb3 (0x2248698) [1116B;1292B)
- instruction: 1212B    %vreg67<def,dead,tied1> =
addh_nimm5_a32_aNhDst_formatDstN32
LLVM ERROR: Found 2 machine code errors.

So if the def operand is tied, then we get a use of the new register
which has no dominating def and the live segment is broken.

==================================================================================
The second report:
==================================================================================
However, now we've found another problem in the same region.

>From the printouts I see:

%vreg29 [880r,896r:0)  0 at 880r
[...]
880B            %vreg29<def> = COPY %a0h
896B            %vreg10<def> = mv_ar16_ar16_lo16In32 %vreg29
[...]
Deleting dead def 896r  %vreg81<def,dead> = mv_ar16_ar16_lo16In32 %vreg29
[...]
Shrink: %vreg29 [880r,896r:0)  0 at 880r
Shrunk: %vreg29 [880r,896r:0)  0 at 880r
[...]

And then the verifier says:

*** Bad machine code: Live segment doesn't end at a valid instruction ***
- function: f3
- basic block: BB#4 bb3 (0x42e13d8) [816B;976B)
- liverange: [880r,896r:0) 0 at 880r
- register: %vreg29
- segment: [880r,896r:0)
LLVM ERROR: Found 1 machine code errors.

Notice the "Shrunk" printout above. The new range is the same as the old one!

Could this be because in LiveRangeEdit::eliminateDeadDef we collect
LiveIntervals in ToShrinkSet &ToShrink, and then someone goes over the
set afterwards and shrinks the LIs now that the dead def is removed.

However, with your changes in eliminateDeadDef the dead def is still
there when attempting the shrinking, so the LI can't be shrunken at
this time.

Then when doing the actual removal of the dead def in postOptimization
we currently don't do anything as far as I can see to update the LIs
of the vregs used in the dead def instruction, and we can end up with
invalid intervals as the verifier says.

On Fri, Apr 22, 2016 at 12:01 PM, Quentin Colombet <qcolombet at apple.com> wrote:
> Hi Wei,
>
> The spill hoisting landed for good now, right?
>
> I basically want to know if the spiller is ready for me to reapply r263460.
>
> Cheers,
> -Quentin
>> On Apr 4, 2016, at 11:50 AM, Wei Mi <wmi at google.com> wrote:
>>
>> On Mon, Apr 4, 2016 at 11:45 AM, Quentin Colombet <qcolombet at apple.com> wrote:
>>>
>>>> On Apr 4, 2016, at 11:43 AM, Wei Mi <wmi at google.com> wrote:
>>>>
>>>> My patch caused llvm bootstrap error on several targets. I am looking
>>>> at it. Will fix the error as soon as possible.
>>>
>>> Do you have a link?
>>>
>>> Q.
>>>
>>
>> Here:
>>
>> bootstrap errors:
>> http://lab.llvm.org:8011/builders/clang-ppc64be-linux-multistage/builds/1681/steps/build%20stage%202/logs/stdio
>> http://lab.llvm.org:8011/builders/clang-3stage-ubuntu/builds/806/steps/build-stage3-clang/logs/stdio
>>
>> test error on aarch64:
>> http://lab.llvm.org:8011/builders/clang-cmake-aarch64-quick/builds/5936/steps/test-suite/logs/test.log
>>
>> Wei.
>