-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
JIT: Port loop unrolling to new loop representation #96454
JIT: Port loop unrolling to new loop representation #96454
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsPort loop unrolling to the new loop representation. Switch strategy slightly with how loop unrolling works:
Some minor diffs are expected:
|
/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress |
Azure Pipelines successfully started running 2 pipeline(s). |
m_dfsTree = fgComputeDfs(); | ||
optFindNewLoops(); | ||
passes++; | ||
goto RETRY_UNROLL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pass could use some factoring into a few separate functions... I'll do that as a mechanical follow-up.
cc @dotnet/jit-contrib PTAL @BruceForstall Diffs. See above for why, in particular when I spot checked most of the ones I hit were the weight change one. |
@@ -4914,6 +4914,8 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl | |||
|
|||
while (iterations > 0) | |||
{ | |||
fgModified = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This used to be set to false by fgComputeDoms
at the end of loop unrolling. It seems better to explicitly do it here, given the comment below on PHASE_OPT_UPDATE_FLOW_GRAPH
.
(It seems a bit questionable not to run the phase if we loop cloned/unrolled -- I saw some beneficial diffs when I originally didn't have this.)
else if (block->bbMemorySsaPhiFunc[memoryKind] == BasicBlock::EmptyMemoryPhiDef) | ||
{ | ||
printf(" = phi([not filled])\n"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've hit crashes here a few times during JITDUMP when I've messed up the flow graph.
optFindNewLoops(); | ||
|
||
fgDomsComputed = false; | ||
fgRenumberBlocks(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why this would be necessary, but there is a preexisting fgDebugCheckBBlist
below that validates sequential bbNum
values. Something for a future PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Port loop unrolling to the new loop representation. Switch strategy slightly with how loop unrolling works:
BBJ_COND
, create a "redirection" block to jump to its fallthrough. This is similar to how loop cloning works and saves a lot of annoying special casing around updating pred lists.fgDfsBlocksAndRemove
. Previously we would create a chain ofBBJ_ALWAYS
going through all the previous blocks, keeping them all reachable, likely because the oldfgComputeDoms
does not handle statically unreachable blocks correctly.Some minor diffs are expected:
BBJ_ALWAYS
, keeping their weight; when we later compact them, we propagate the original "loop" weight to the blocks we compact with. This causes differences in if-conversion, register allocation and block layout.