[LLVMdev] Assert in LiveInterval update

[Sergei Larin]

Andy, 

  I've described that issue (see below) when you were out of town... I think
I am getting more context on it. Please take a look...

So, in short, when the new MI scheduler performs move of an instruction, it
does something like this:

    // Move the instruction to its new location in the instruction stream.
    MachineInstr *MI = SU->getInstr();

    if (IsTopNode) {
      assert(SU->isTopReady() && "node still has unscheduled dependencies");
      if (&*CurrentTop == MI)              <<<<<<<<<<<<<<<<<<  Here we make
sure that CurrentTop != MI. 
        CurrentTop = nextIfDebug(++CurrentTop, CurrentBottom);
      else {
        moveInstruction(MI, CurrentTop);
        TopRPTracker.setPos(MI);
      }
...

But in moveInstruction we use 

  // Update the instruction stream.
  BB->splice(InsertPos, BB, MI);

And splice as far as I understand moves MI to the location right __before__
InsertPos. Our previous check made sure that InsertPos != MI, 
But I do hit a case, when MI == InsertPos--, and effectively tries to swap
instruction with itself... which make live update mechanism very unhappy.

If I am missing something in intended logic, please explain, otherwise it
looks like we need to adjust that check to make sure we never even
considering this situation (swapping with self).

I also wonder if we want explicit check in live update with more meaningful
assert :)

Thanks.

Sergei


--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum.

> -----Original Message-----
> From: llvmdev-bounces at cs.uiuc.edu [mailto:llvmdev-bounces at cs.uiuc.edu]
> On Behalf Of Sergei Larin
> Sent: Friday, August 17, 2012 3:32 PM
> To: 'Andrew Trick'; 'Jakob Olesen'
> Cc: 'LLVM Developers Mailing List'
> Subject: [LLVMdev] Assert in LiveInterval update
> 
> 
> Andy, Jacob,
> 
>   I have ported Hexagon MI scheduler to use the new scheduler
> infrastructure, but one of my tests triggers an assert in LiveInterval
> update. On the surface it does not make much sense to me, so I wonder
> if this is something you readily recognize, before I try to prop it
> open...
> 
> The assert is:
> 
> lib/CodeGen/LiveInterval.cpp:266: llvm::LiveRange*
> llvm::LiveInterval::addRangeFrom(llvm::LiveRange, llvm::LiveRange*):
> Assertion `B->end <= Start && "Cannot overlap two LiveRanges with
> differing ValID's" " (did you def the same reg twice in a
> MachineInstr?)"' failed.
> 
> has this following call stack:
> 
> #3  in llvm::LiveInterval::addRangeFrom (this=0x46e99a0, LR=...,
> From=0x46ee4b0) at lib/CodeGen/LiveInterval.cpp:266
> #4  in llvm::LiveInterval::addRange (this=0x46e99a0, LR=...) at
> include/llvm/CodeGen/LiveInterval.h:384
> #5  in llvm::LiveIntervals::HMEditor::moveInternalFrom
> (this=0x7fffffff8010, OldIdx=..., P=...)  at
> lib/CodeGen/LiveIntervalAnalysis.cpp:1220
> #6  in llvm::LiveIntervals::HMEditor::moveAllInternalFrom
> (this=0x7fffffff8010, OldIdx=..., Internal=...)  at
> lib/CodeGen/LiveIntervalAnalysis.cpp:1226
> #7  in llvm::LiveIntervals::HMEditor::moveAllRangesFrom
> (this=0x7fffffff8010, MI=0x462ab80, OldIdx=...)  at
> lib/CodeGen/LiveIntervalAnalysis.cpp:938
> #8  in llvm::LiveIntervals::handleMove (this=0x448b6f0, MI=0x462ab80)
> at
> lib/CodeGen/LiveIntervalAnalysis.cpp:1388
> #9  in llvm::VLIWMachineScheduler::moveInstruction (this=0x46b0f50,
> MI=0x462ab80, InsertPos=...)    at
> lib/Target/Hexagon/HexagonMachineScheduler.cpp:120
> #10 in llvm::VLIWMachineScheduler::schedule (this=0x46b0f50) at
> lib/Target/Hexagon/HexagonMachineScheduler.cpp:378
> #11 in (anonymous namespace)::MachineScheduler::runOnMachineFunction
> (this=0x448c730, mf=...)   at lib/CodeGen/MachineScheduler.cpp:263
> #12 in llvm::MachineFunctionPass::runOnFunction (this=0x448c770, F=...)
> at
> lib/CodeGen/MachineFunctionPass.cpp:33
> 
> 
> For the purpose of this question, you can assume
> HexagonMachineScheduler.cpp == MachineScheduler.cpp and
> VLIWMachineScheduler == ScheduleDAGMI
> 
> 
> The instruction being moved is a simple call:
> 
> let isCall = 1, neverHasSideEffects = 1,
>   Defs = [D0, D1, D2, D3, D4, D5, D6, D7, R28, R31,
>           P0, P1, P2, P3, LC0, LC1, SA0, SA1, USR] in {
>   def CALLv3 : JInst<(outs), (ins calltarget:$dst),
>              "call $dst", []>, Requires<[HasV3T]>; }
> 
> CALLv3 <ga:@printf>, %D0<imp-def,dead>, %D1<imp-def,dead>, %D2<imp-
> def,dead>, %R31<imp-def>, %R0<imp-use,kill>, ...
> 
> Another clue - slot renumbering just took place:
> 
> *** Renumbered SlotIndexes 1056-2120 *** ...
> 
> *** Renumbered SlotIndexes 1068-2140 ***
> 
> ...and the first move after that produces the assert.
> 
> (gdb) p Start.dump()
> 1080r
> (gdb) p B->end.dump()
> 1092r
> (gdb) p *LR.valno
> $7 = {id = 61, def = {lie = {Value = 74160402}}}
> (gdb) p *B->valno
> $9 = {id = 0, def = {lie = {Value = 73472560}}}
> 
> I have not yet succeeded to reproduce it on anything other than my
> branch of Hexagon, but I can try...
> This is definitely some corner case, since it is a singular failure in
> a very large test suite. Really hope it rings a bell for you.
> 
> Thanks.
> 
> Sergei
> 
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum.
> 
> 
> 
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu         http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev