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

JIT: Set bbFalseTarget upon BBJ_COND initialization/modification #96265

Merged
merged 8 commits into from
Jan 1, 2024

Conversation

amanasifkhalid
Copy link
Member

Part of #93020. Previously, bbFalseTarget was hard-coded to match bbNext in BasicBlock::SetNext. We still require bbFalseTarget to point to the next block for BBJ_COND blocks, but I've removed the logic for updating bbFalseTarget from SetNext, and placed calls to SetFalseTarget wherever bbFalseTarget needs to be updated because the BBJ_COND block has been created or moved relative to its false successor. This helps set us up to start removing logic that enforces the block's false successor is the next block.

@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 Dec 22, 2023
@ghost ghost assigned amanasifkhalid Dec 22, 2023
@ghost
Copy link

ghost commented Dec 22, 2023

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

Issue Details

Part of #93020. Previously, bbFalseTarget was hard-coded to match bbNext in BasicBlock::SetNext. We still require bbFalseTarget to point to the next block for BBJ_COND blocks, but I've removed the logic for updating bbFalseTarget from SetNext, and placed calls to SetFalseTarget wherever bbFalseTarget needs to be updated because the BBJ_COND block has been created or moved relative to its false successor. This helps set us up to start removing logic that enforces the block's false successor is the next block.

Author: amanasifkhalid
Assignees: amanasifkhalid
Labels:

area-CodeGen-coreclr

Milestone: -

@amanasifkhalid
Copy link
Member Author

cc @dotnet/jit-contrib. No asmdiffs, and small TP diffs. SPMI failures are due to timeouts on win-x86.

I noticed that we only use Compiler::fgConnectFallThrough to connect BBJ_COND blocks to their fallthrough successors with a new BBJ_ALWAYS successor; this strategy doesn't work for connecting BBJ_CALLFINALLY blocks to their successors since Bruce introduced BBJ_CALLFINALLYRET. When inserting/moving blocks, we already avoid splitting up BBJ_CALLFINALLY/BBJ_CALLFINALLYRET pairs, and retless BBJ_CALLFINALLY blocks shouldn't fall through, so I don't think we'd expect to encounter a BBJ_CALLFINALLY block in Compiler::fgConnectFallThrough. Thus, I've cleaned this method up a bit.

// TODO-NoFallThrough: also set bbFalseTarget
bbKind = BBJ_COND;
bbTrueTarget = trueTarget;
bbFalseTarget = bbNext;
Copy link
Member

Choose a reason for hiding this comment

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

Should this function take a falseTarget parameter that is used to set bbFalseTarget? Then, callers would be required to set it. Maybe later on when we remove bbNext as fall through?

Copy link
Member Author

Choose a reason for hiding this comment

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

That makes sense to me. I'll open a follow-up PR with that refactor; it might help others avoid hitting asserts around bbFalseTarget by forcing them to set it when initializing bbTrueTarget.

Thank you for the review!

@amanasifkhalid amanasifkhalid merged commit 25efee0 into dotnet:main Jan 1, 2024
124 of 129 checks passed
amanasifkhalid added a commit that referenced this pull request Jan 3, 2024
…96412)

Fixes #96391. In #96265, I neglected to set the false target for BBJ_COND blocks preceding the first cold block when inserting a fixup block between the two
@amanasifkhalid amanasifkhalid deleted the bbj-cond-diverge branch January 3, 2024 16:28
@github-actions github-actions bot locked and limited conversation to collaborators Feb 3, 2024
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.

2 participants