[LLVMdev] Assert in LiveInterval update

[Andrew Trick]

On Aug 28, 2012, at 8:18 AM, Sergei Larin <slarin at codeaurora.org> wrote:
> 
>  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 for debugging this! The code above assumes that you're moving an unscheduled instruction, which should never be above InsertPos for top-down scheduling. If the instruction was in multiple ready Q's, then you may attempt to schedule it multiple times. You can avoid this by checking Su->isScheduled in your Strategy's pickNode. See InstructionShuffler::pickNode for an example. I don't see an equivalent check in ConvergingScheduler, but there probably should be.

Another possibility to consider is something strange with DebugValues, which I haven't tested much.

I reproduced the same assert on arm and filed PR13719. I'm not sure yet if it's exactly the same issue, but we can move the discussion there.

We need a better assert in live update and probably the scheduler too. Lang mentioned he may look at the issue today. Meanwhile, I hope my suggestion above helps.

-Andy