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

Don't run optJumpThreading when fgCurBBEpochSize != (fgBBNumMax + 1) #72440

Merged
merged 4 commits into from
Jul 19, 2022

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Jul 19, 2022

Fixes #71599

@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 Jul 19, 2022
@ghost ghost assigned EgorBo Jul 19, 2022
@ghost
Copy link

ghost commented Jul 19, 2022

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

Issue Details

Fixes #71599

Author: EgorBo
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@EgorBo
Copy link
Member Author

EgorBo commented Jul 19, 2022

So the problem that optLoopHoist phase creates a new block here, it invalidates the assumption that fgCurBBEpochSize == (fgBBNumMax + 1)

optJumpThreading seems to not like it when it does BlockSetOps::AddElemD under the hood.

It seems to me that we can't just re-calculate doms and renumber blocks in-between loopHoist and optRedundantBranch phases (due to SSA state?)

so this fix works but it has some diffs. What do you think @AndyAyersMS @BruceForstall

if (doLoopHoisting)
{
// Hoist invariant code out of loops
//
DoPhase(this, PHASE_HOIST_LOOP_CODE, &Compiler::optHoistLoopCode);
}
if (doCopyProp)
{
// Perform VN based copy propagation
//
DoPhase(this, PHASE_VN_COPY_PROP, &Compiler::optVnCopyProp);
}
if (doBranchOpt)
{
DoPhase(this, PHASE_OPTIMIZE_BRANCHES, &Compiler::optRedundantBranches);
}

@EgorBo EgorBo marked this pull request as ready for review July 19, 2022 14:17
@EgorBo EgorBo changed the title Update basicblock epoch in bbNewBasicBlock Don't run optJumpThreading when fgCurBBEpochSize != (fgBBNumMax + 1) Jul 19, 2022
Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

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

Seems like an appropriate fix for now.

We really need to start eagerly creating preheaders, so hoisting can stop modifying the flow graph.

// We added new blocks since the last renumerate e.g. in optLoopHoist
break;
}

Copy link
Member

Choose a reason for hiding this comment

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

I would put this check down at the start of optJumpThread.

Copy link
Member

Choose a reason for hiding this comment

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

optJumpThread doesn't really depend on the epoch, it depends onBlockSet which depends on the epoch.

Down the road we should consider giving optJumpThread alternative ways of keeping track of the different sets of preds, I only used BlockSet here because it seemed convenient. The pred sets are likely going to be pretty small (most blocks don't have many preds) so something simpler might actually both remove this dependency and be more efficient overall.

I'll add a note to #48115.

@EgorBo
Copy link
Member Author

EgorBo commented Jul 19, 2022

Failure is #72429

@EgorBo EgorBo merged commit 28c56fe into dotnet:main Jul 19, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Aug 19, 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 'i < BitSetTraits::GetSize(env)' in during 'Redundant branch opts'
2 participants