Skip to content

Conversation

@EgorBo
Copy link
Member

@EgorBo EgorBo commented Jul 19, 2022

Fixes #71599

@ghost ghost 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