Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reset the bbNatLoopNum of block for removed loop #68804

Merged
merged 5 commits into from
May 11, 2022

Conversation

kunalspathak
Copy link
Member

When a loop is removed, we should also reset the bbNatLoopNum present at the blocks to reflect the updated information.

Fixes: #68769

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label May 3, 2022
@ghost ghost assigned kunalspathak May 3, 2022
@kunalspathak
Copy link
Member Author

@dotnet/jit-contrib

@ghost
Copy link

ghost commented May 3, 2022

Tagging subscribers to this area: @JulieLeeMSFT
See info in area-owners.md if you want to be subscribed.

Issue Details

When a loop is removed, we should also reset the bbNatLoopNum present at the blocks to reflect the updated information.

Fixes: #68769

Author: kunalspathak
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@kunalspathak
Copy link
Member Author

I intentionally added an assert about assert(auxBlock->bbNatLoopNum == loopNum); and realized that this is not always true and we hit the assert even while compiling SPC because auxBlock could be inside a nested loop and bbNatLoopNum could be pointing to that.

But I also realized that we don't update the lpParent field of child loops if a parent is removed and that is one of the reason we have this:

if (optAnyChildNotRemoved(loopNum))
{
JITDUMP("Removed loop " FMT_LP " has one or more live children\n", loopNum);
}

// optAnyChildNotRemoved: Recursively check the child loops of a loop to see if any of them
// are still live (that is, not marked as LPFLG_REMOVED). This check is done when we are
// removing a parent, just to notify that there is something odd about leaving a live child.

If we have live children, at the minimal, we should at least update the lpParent to record that lpParent is no longer its parent.

Comment on lines 9602 to 9612
for (BasicBlock::loopNumber l = optLoopTable[loopNum].lpChild; //
l != BasicBlock::NOT_IN_LOOP; //
l = optLoopTable[l].lpSibling)
{
if ((optLoopTable[l].lpFlags & LPFLG_REMOVED) == 0)
{
JITDUMP("Resetting parent of loop number " FMT_LP " from " FMT_LP " to " FMT_LP ".\n", l,
optLoopTable[l].lpParent, optLoopTable[loopNum].lpParent);
optLoopTable[l].lpParent = optLoopTable[loopNum].lpParent;
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
for (BasicBlock::loopNumber l = optLoopTable[loopNum].lpChild; //
l != BasicBlock::NOT_IN_LOOP; //
l = optLoopTable[l].lpSibling)
{
if ((optLoopTable[l].lpFlags & LPFLG_REMOVED) == 0)
{
JITDUMP("Resetting parent of loop number " FMT_LP " from " FMT_LP " to " FMT_LP ".\n", l,
optLoopTable[l].lpParent, optLoopTable[loopNum].lpParent);
optLoopTable[l].lpParent = optLoopTable[loopNum].lpParent;
}
}
for (BasicBlock::loopNumber l = loop.lpChild; //
l != BasicBlock::NOT_IN_LOOP; //
l = optLoopTable[l].lpSibling)
{
if ((optLoopTable[l].lpFlags & LPFLG_REMOVED) == 0)
{
JITDUMP("Resetting parent of loop number " FMT_LP " from " FMT_LP " to " FMT_LP ".\n", l,
optLoopTable[l].lpParent, loop.lpParent);
optLoopTable[l].lpParent = loop.lpParent;
}
}

@kunalspathak kunalspathak merged commit cb2277d into dotnet:main May 11, 2022
@kunalspathak kunalspathak deleted the loop_removed_state branch May 11, 2022 23:19
@ghost ghost locked as resolved and limited conversation to collaborators Jun 11, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Assertion failed '(l2 < optLoopCount) || (l2 == BasicBlock::NOT_IN_LOOP)' during 'Do value numbering'
2 participants