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: Add explicit block fallthrough successor #93772

Closed
wants to merge 3 commits into from

Conversation

amanasifkhalid
Copy link
Member

@amanasifkhalid amanasifkhalid commented Oct 20, 2023

Next step for #93020. This change shouldn’t introduce any behavioral changes, as we don’t allow the fall-through successor to diverge from bbNext yet (that will be in a follow-up PR). Once we allow these to diverge (plus disallow BBJ_NONE until the block layout is set), we’ll theoretically no longer have to maintain an invariant of contiguous fall-through successors during block reordering, opening up more ordering possibilities.

Note that while BBJ_CALLFINALLY blocks can fall through, we don’t plan to allow their fall-through successors to diverge from bbNext yet. We will have to determine if there is any benefit in splitting up call-always pairs first.

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

ghost commented Oct 20, 2023

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

Issue Details

Next step for #93020. This change shouldn’t introduce any behavioral changes, as we don’t allow the fall-through successor to diverge from bbNext yet (that will be in a follow-up PR). Once we allow these to diverge (plus disallow BBJ_NONE until the block layout is set), we’ll theoretically no longer have to maintain an invariant of contiguous fall-through successors during block reordering, opening up more ordering possibilities.

Note that while BBJ_CALLFINALLY blocks can fall through, we don’t plan to allow their fall-through successors to diverge from bbNext yet. We will have to determine if there is any benefit in splitting up call-finally pairs first.

Author: amanasifkhalid
Assignees: amanasifkhalid
Labels:

area-CodeGen-coreclr

Milestone: -

@amanasifkhalid
Copy link
Member Author

cc @dotnet/jit-contrib, @AndyAyersMS PTAL. superpmi-replay failure is #93527

@@ -666,6 +669,38 @@ struct BasicBlock : private LIR::Range
SetJumpKindAndTarget(jumpKind, jumpDest DEBUG_ARG(compiler));
}

BasicBlock* GetFallThroughSucc() const
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit of a confusing name. The fall-through successor is always going to be the next block.

What may not be is the "false" branch of a BBJ_COND block / JTRUE node. Is this meant to model that, or...?

Copy link
Member Author

@amanasifkhalid amanasifkhalid Oct 20, 2023

Choose a reason for hiding this comment

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

Yes, in the case of BBJ_COND blocks, this would be the destination of the false branch. I wanted this name to capture the idea that, in the case of BBJ_COND blocks, bbFallThroughSucc will be taken if bbJumpDest isn't, and that this fall-through successor isn't necessarily the next block (which means the control flow doesn't actually "fall through"). Do you have any suggestions for the name @SingleAccretion? I don't want it to sound specific to BBJ_COND since we might decide to allow BBJ_CALLFINALLY/BBJ_ALWAYS pairs to be non-contiguous later, thus making bbFallThroughSucc relevant.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest GetNormalJumpDest() for symmetry with GetJumpDest(). It matches the expected codegen for BBJ_COND:

jcc <cond jump dest>
jmp <normal jump dest>

I wouldn't worry too much about the callfinally cases. Callfinally cases are inherently complex, one has to look up how they work specifically. Even if you make the BBJ_ALWAYS part of the pair non-contiguous with the callfinally, you'll still not have it as a "successor" in the flowgraph sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see, thanks for the suggestion! I'll change it.

Comment on lines 684 to 685
// BBJ_CALLFINALLY can fall through too, but we don't allow its fallthrough successor to diverge from bbNext
// because we aren't interested in splitting up call-always pairs
Copy link
Contributor

@SingleAccretion SingleAccretion Oct 20, 2023

Choose a reason for hiding this comment

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

This is a confusing comment. BBJ_CALLFINALLYs never fall through, as far the flowgraph is concerned - they always "jump" to their corresponding finally handler. The handler then "jumps back" to the BBJ_ALWAYS part of the pair via BBJ_EHFINALLYRET.

(Physically this can be implemented as a call, so it looks a bit like BBJ_CALLFINALLY falls through to BBJ_ALWAYS, but that's a very low-level view)

Copy link
Member Author

Choose a reason for hiding this comment

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

I used the "fall through" language there based on the fact that BasicBlock::bbFallsThrough returns true for BBJ_CALLFINALLY blocks part of BBJ_CALLFINALLY/BBJ_ALWAYS pairs, though I see how describing these blocks as falling through isn't wholly accurate. I can clarify that comment; something like "For BBJ_CALLFINALLY blocks part of call-always pairs, bbFallThroughSucc points to the BBJ_ALWAYS block. For now, call-always pairs are always contiguous, so bbFallThroughSucc cannot diverge from bbNext."

@amanasifkhalid
Copy link
Member Author

No asmdiffs, but some TP diffs from having to set bbFallThroughSucc alongside bbNext.

@amanasifkhalid
Copy link
Member Author

amanasifkhalid commented Oct 20, 2023

FYI: looks like [skip ci] in the commit message doesn't skip CI in dotnet/runtime... @hoyosjs

Comment on lines 1280 to +1281
case BBJ_NONE:
return bbNext;
return GetFallThroughSucc();
Copy link
Contributor

@SingleAccretion SingleAccretion Oct 20, 2023

Choose a reason for hiding this comment

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

Here and in a number of other places in this change, the pattern seems to be to change: BBJ_NONE -> bbNext / BBJ_COND -> bbNext+bbJumpDest to BBJ_NONE -> GetFallThroughSucc() / BBJ_COND -> GetFallThroughSucc()+bbJumpDest. The BBJ_COND part makes sense, but the BBJ_NONE changes confuse me.

If GetFallThroughSucc() is the 'normal' successor of BBJ_COND, why touch BBJ_NONE?

Side note: I think you will also want to update VisitAllSuccs/VisitRegularSuccs.

Copy link
Member Author

@amanasifkhalid amanasifkhalid Oct 20, 2023

Choose a reason for hiding this comment

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

I'm not sure what @AndyAyersMS's plan is here, but a "lazy" approach to reordering the block layout might be moving a block without considering it might be a BBJ_NONE and its successor is its old bbNext, and just converting the BBJ_NONE to BBJ_ALWAYS after the reorder. If we do something like this, I think it would be nice to always use GetFallThroughSucc for BBJ_NONE blocks so we can assert the fall-through successor matches bbNext; if we mess something up during block reordering, hopefully this will help us catch it.

Side note: I think you will also want to update VisitAllSuccs/VisitRegularSuccs.

Thanks for catching that. I'll fix those.

Copy link
Contributor

@SingleAccretion SingleAccretion Oct 20, 2023

Choose a reason for hiding this comment

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

I feel there is some overloading of meanings for "the fall-through successor" here.

The change at large is needed because in the refactored flowgraph, the "false" successor of BBJ_COND (and possibly the BBJ_ALWAYS part of callfinally pairs) will be distinct from bbNext. So, overall, the imported blocks will change as follows:

  1. BBJ_NONE -> bbNext -> BBJ_ALWAYS -> bbJumpDest (can be reordered arbitrarily later on).
  2. BBJ_COND -> bbNext, bbJumpDest -> BBJ_COND -> bbTheNewFieldBeingAdded, bbJumpDest (same).

In other words, "fall through" will not exist as an explicit flowgraph concept like it does today (perhaps until after a very late layout stage, where BBJ_NONE may appear as an optional optimization).

In that light, I would expect changes to basically replace case BBJ_COND: if (<false branch>) return bbNext; with case BBJ_COND: if (<false branch>) return bbTheNewFieldBeingAdded;, and indeed they do in some places like switch recognition, but not in others.

Copy link
Member

Choose a reason for hiding this comment

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

My initial plan was to disallow BBJ_NONE until we'd set block layout. I think we'll have to chip away at this as we have a lot of code that needs to work both before and after we do layout.

So a plausible starting point is to allow BBJ_NONE but treat it somewhat like BBJ_ALWAYS, meaning it has to explicitly name its flow successor, and find some way to start incrementally removing the early BBJ_NONE cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if simply not producing BBJ_NONE (anywhere), instead producing BBJ_ALWAYS, and adding a small peephole to codegen to not emit "branch to next block" would be a faster/easier way to go about this.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wonder if simply not producing BBJ_NONE (anywhere), instead producing BBJ_ALWAYS, and adding a small peephole to codegen to not emit "branch to next block" would be a faster/easier way to go about this.

With this approach, maybe we can get rid of bbFallThroughSucc and instead add a new member specific to BBJ_COND for its false branch target.

@hoyosjs
Copy link
Member

hoyosjs commented Oct 20, 2023

FYI: looks like [skip ci] in the commit message doesn't skip CI in dotnet/runtime... @hoyosjs

It's documented in https://learn.microsoft.com/en-us/azure/devops/pipelines/repos/azure-repos-git?view=azure-devops&tabs=yaml#skipping-ci-for-individual-pushes and there's so much "caveat" that seems broken by design for PR. In particular:

The pipelines specified by the target branch's build validation policy will run on the merge commit (the merged code between the source and target branches of the pull request), regardless if there exist pushed commits whose messages or descriptions contain [skip ci] (or any of its variants).

Looks like it has a long history of how broken it is for GH / AzDO PRs and it's claimed to be by design. :( microsoft/azure-pipelines-agent#858 microsoft/azure-pipelines-agent#2944. If people think there's enough value, we can easily add something to the evaluate paths step to skip with the string (@agocke and @jkoritzinsky, thoughts on such check for PRs).

@amanasifkhalid
Copy link
Member Author

Looks like it has a long history of how broken it is for GH / AzDO PRs and it's claimed to be by design.

I see, thank you for digging into this!

If people think there's enough value, we can easily add something to the evaluate paths step to skip with the string

+1, I think that would be a great quality-of-life improvement, particularly for trivial style/comment updates to PRs that touch source code.

@amanasifkhalid
Copy link
Member Author

@AndyAyersMS should we put this change on hold for now, and I can try removing BBJ_NONE altogether? Jakob seems interested in that approach, too.

@AndyAyersMS
Copy link
Member

I can try removing BBJ_NONE altogether?

Sure, give it a try and see what happens.

amanasifkhalid added a commit that referenced this pull request Nov 28, 2023
Next step for #93020, per conversation on #93772. Replacing BBJ_NONE with BBJ_ALWAYS to the next block helps limit our use of implicit fall-through (though we still expect BBJ_COND to fall through when its false branch is taken; #93772 should eventually address this).

I've added a small peephole optimization to skip emitting unconditional branches to the next block during codegen.
@amanasifkhalid
Copy link
Member Author

Closing in favor of #95773.

@github-actions github-actions bot locked and limited conversation to collaborators Jan 7, 2024
@amanasifkhalid amanasifkhalid deleted the fallthrough branch January 25, 2024 18:02
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.

4 participants