JIT: Add explicit block fallthrough successor by amanasifkhalid · Pull Request #93772 · dotnet/runtime

amanasifkhalid · 2023-10-20T11:43:16Z

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 · 2023-10-20T11:43:29Z

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 · 2023-10-20T18:00:30Z

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

SingleAccretion · 2023-10-20T17:58:23Z

src/coreclr/jit/block.h

        SetJumpKindAndTarget(jumpKind, jumpDest DEBUG_ARG(compiler));
    }

+    BasicBlock* GetFallThroughSucc() const


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...?

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.

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.

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

SingleAccretion · 2023-10-20T18:02:53Z

src/coreclr/jit/block.h

+        // 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


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)

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 · 2023-10-20T18:55:35Z

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

amanasifkhalid · 2023-10-20T19:05:36Z

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

SingleAccretion · 2023-10-20T19:14:00Z

src/coreclr/jit/block.cpp

        case BBJ_NONE:
-            return bbNext;
+            return GetFallThroughSucc();


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.

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.

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:

BBJ_NONE -> bbNext -> BBJ_ALWAYS -> bbJumpDest (can be reordered arbitrarily later on).

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.

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.

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.

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 · 2023-10-20T22:32:18Z

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 · 2023-10-20T22:42:49Z

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 · 2023-10-23T15:59:59Z

@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 · 2023-10-23T16:20:27Z

I can try removing BBJ_NONE altogether?

Sure, give it a try and see what happens.

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 · 2023-12-07T23:54:37Z

Closing in favor of #95773.

ghost assigned amanasifkhalid Oct 20, 2023

ghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Oct 20, 2023

amanasifkhalid added 2 commits October 20, 2023 11:37

Add explicit block fallthrough successor

f841d28

Fix assert

9acad86

amanasifkhalid force-pushed the fallthrough branch from 45b4b9a to 9acad86 Compare October 20, 2023 15:37

amanasifkhalid mentioned this pull request Oct 20, 2023

JIT: Flow Graph Modernization and Improved Block Layout #93020

Closed

51 tasks

SingleAccretion reviewed Oct 20, 2023

View reviewed changes

[skip ci] Update SetFallThroughSucc comment

a1ce47e

SingleAccretion reviewed Oct 20, 2023

View reviewed changes

amanasifkhalid mentioned this pull request Oct 23, 2023

JIT: Set bbJumpKind and bbJumpDest during block initialization #93415

Merged

amanasifkhalid mentioned this pull request Oct 31, 2023

JIT: Remove BBJ_NONE #94239

Merged

amanasifkhalid closed this Dec 7, 2023

SingleAccretion mentioned this pull request Dec 8, 2023

JIT: Add explicit successor for BBJ_COND false branch #95773

Merged

github-actions bot locked and limited conversation to collaborators Jan 7, 2024

amanasifkhalid deleted the fallthrough branch January 25, 2024 18:02

		// 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

Conversation

amanasifkhalid commented Oct 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Oct 20, 2023

Uh oh!

amanasifkhalid commented Oct 20, 2023

Uh oh!

SingleAccretion Oct 20, 2023

Choose a reason for hiding this comment

Uh oh!

amanasifkhalid Oct 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SingleAccretion Oct 20, 2023

Choose a reason for hiding this comment

Uh oh!

amanasifkhalid Oct 20, 2023

Choose a reason for hiding this comment

Uh oh!

SingleAccretion Oct 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amanasifkhalid Oct 20, 2023

Choose a reason for hiding this comment

Uh oh!

amanasifkhalid commented Oct 20, 2023

Uh oh!

amanasifkhalid commented Oct 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SingleAccretion Oct 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amanasifkhalid Oct 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SingleAccretion Oct 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndyAyersMS Oct 20, 2023

Choose a reason for hiding this comment

Uh oh!

SingleAccretion Oct 20, 2023

Choose a reason for hiding this comment

Uh oh!

amanasifkhalid Oct 20, 2023

Choose a reason for hiding this comment

Uh oh!

hoyosjs commented Oct 20, 2023

Uh oh!

amanasifkhalid commented Oct 20, 2023

Uh oh!

amanasifkhalid commented Oct 23, 2023

Uh oh!

AndyAyersMS commented Oct 23, 2023

Uh oh!

amanasifkhalid commented Dec 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

amanasifkhalid commented Oct 20, 2023 •

edited

Loading

amanasifkhalid Oct 20, 2023 •

edited

Loading

SingleAccretion Oct 20, 2023 •

edited

Loading

amanasifkhalid commented Oct 20, 2023 •

edited

Loading

SingleAccretion Oct 20, 2023 •

edited

Loading

amanasifkhalid Oct 20, 2023 •

edited

Loading

SingleAccretion Oct 20, 2023 •

edited

Loading