Workaround for compiler.hpp (1848) - Assertion failed 'lvRefCnt'

[sandreenko]

The description is in #18291.

The fix is to replace JTRUE relop before morph the list of its side effects. In this case when we have fgRemoveRestOfBlock and call fgRemoveStmt it removes updated JTRUE and this part works correct.

The problem that optVNConstantPropCurStmt could receive a statement that was already removed still exists.

[sandreenko]

ci example: Windows_NT x64 Checked Innerloop Build and Test

[sandreenko]

PTAL @dotnet/jit-contrib

[sandreenko]

@dotnet-bot test Windows_NT x64 Checked jitstress2
@dotnet-bot test Windows_NT x86 Checked jitstress2

[AndyAyersMS]

Our ambition is to remove all incremental ref count maintenance -- in part because of issues like this -- in the not too distant future.

So if this is some kind of artificial bug case, perhaps we can just hold off fixing it for now?

[AndyAyersMS]

Going to re-trigger the formatting tests to make sure my recent jitutils update is OK.

@dotnet-bot test Windows_NT x64 Formatting

[AndyAyersMS]

Yep, looks to be ok:

08:16:01   Restore completed in 41.58 ms for D:\j\workspace\x64_windows_n---616f30f2\jitutils\src\pmi\PMI.csproj.
08:16:02   PMI -> D:\j\workspace\x64_windows_n---616f30f2\jitutils\src\pmi\bin\win81-x64\Release\netcoreapp2.1\PMI.dll
08:16:02   PMI -> D:\j\workspace\x64_windows_n---616f30f2\jitutils\bin\
08:16:02         1 file(s) copied.

[sandreenko]

So if this is some kind of artificial bug case

Yes, it is an artificial bug case, however, I think it can be recreated with C# code.

Our ambition is to remove all incremental ref count maintenance -- in part because of issues like this -- in the not too distant future.

Do we have an issue to track that ambition?
The problem is not only that we decrease counters twice, but also that we remove other trees twice. The PR makes things better, we replace JTRUE relop with a new created tree and then start extracting and morphing side effects, it means we do not have a moment when IR has duplicates, as it was before (JTRUE has old relop tree and part of it was already extracted and inserted before JTRUE). So we need this change even after removing all incremental ref count maintenance.

perhaps we can just hold off fixing it for now?

I do not see why we should hold it.

[sandreenko]

Could somebody pls take a look, @dotnet/jit-contrib ?

[AndyAyersMS]

Issue for incremental ref count removal is #13280.

The change you made looks ok but it rreallly needs a comment saying why things must be done in this particular order as it's not obvious.

I wonder how many other places we may have similar ordering issues and haven't hit the right test case yet?

I'm also surprised we don't hit other asserts when trying to remove trees twice. Seems like we should be calling DEBUG_DESTROY_NODE in fgRemoveStmt. Though I recall being confused as to what the expected behavior of this method was in the past (#8059)-- it seemingly can't destroy everything in the statement. But perhaps it can destroy the statement expression?

[sandreenko]

I'm also surprised we don't hit other asserts when trying to remove trees twice. Seems like we should be calling DEBUG_DESTROY_NODE in fgRemoveStmt. Though I recall being confused as to what the expected behavior of this method was in the past (#8059)-- it seemingly can't destroy everything in the statement. But perhaps it can destroy the statement expression?

It was my first idea :-), but it appeared that you can't do it, because we usefgRemoveStmt to move statements, like fgRemoveStmt from here and fgInsertStmt there. I thought about adding a debug flag removed and check it there, but I think it should be a separate change.

Yes, I think it would be a good idea to separate remove for move and remove as delete and destroy.

[sandreenko]

The change you made looks ok but it rreallly needs a comment saying why things must be done in this particular order as it's not obvious.

Sounds reasonable, I will think how to describe it better.

[briansull]

Looks Good

[sandreenko]

I have added 2 comments to explain it better.

I think we should add a separate issue to better check and assert flowGraph transformations.

For example in the existing code when JTRUE has two side effects, THROW and something else, and THROW goes first, that code

    while (sideEffList != nullptr) <- sideEffList = {THROW, smth else}
    {
        GenTree* newStmt;
        if (sideEffList->OperGet() == GT_COMMA)
        {
            newStmt     = fgInsertStmtNearEnd(block, sideEffList->gtGetOp1());
            sideEffList = sideEffList->gtGetOp2();
        }
        else
        {
            newStmt     = fgInsertStmtNearEnd(block, sideEffList); <-- will delete stmts after THROW, including JTRUE on the first iteration.
            sideEffList = nullptr;
        }
        // fgMorphBlockStmt could potentially affect stmts after the current one,
        // for example when it decides to fgRemoveRestOfBlock.
        fgMorphBlockStmt(block, newStmt->AsStmt() DEBUGARG(__FUNCTION__));
    }

on the second iteration something else will call fgInsertStmtNearEnd and the statement will be inserted after THROW without any need.

[AndyAyersMS]

Thanks for the comment. Looks good.