JIT: Add explicit successor for BBJ_COND false branch #95773

amanasifkhalid · 2023-12-07T23:54:16Z

Part of #93020. Currently, if a BBJ_COND block's bbJumpDest branch is not taken, control falls through into the next block. Because we plan to remove this fall-through requirement, BBJ_COND blocks need a pointer to their "not-taken" branch target, as the target may no longer be the next block. This new pointer, bbNormalJumpDest, may also be used in the future to represent the finally continuation of a BBJ_CALLFINALLY/BBJ_ALWAYS pair, once this pattern is consolidated into one block with no implicit fall-through via #95355.

For now, bbNormalJumpDest models current behavior by always pointing to the next block. Follow-up work will introduce behavioral changes by allowing bbNormalJumpDest to diverge from bbNext.

ghost · 2023-12-07T23:54:27Z

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

Issue Details

Part of #93020. Currently, if a BBJ_COND block's bbJumpDest branch is not taken, control falls through into the next block. Because we plan to remove this fall-through requirement, BBJ_COND blocks need a pointer to their "not-taken" branch target, as the target may no longer be the next block. This new pointer, bbNormalJumpDest, may also be used in the future to represent the finally continuation of a BBJ_CALLFINALLY/BBJ_ALWAYS pair, once this pattern is consolidated into one block with no implicit fall-through via #95355.

For now, bbNormalJumpDest models current behavior by always pointing to the next block. Follow-up work will introduce behavioral changes by allowing bbNormalJumpDest to diverge from bbNext.

Author:	amanasifkhalid
Assignees:	amanasifkhalid
Labels:	`area-CodeGen-coreclr`
Milestone:	-

amanasifkhalid · 2023-12-08T01:05:36Z

src/coreclr/jit/block.h

@@ -573,11 +576,18 @@ struct BasicBlock : private LIR::Range

    void SetNext(BasicBlock* next)
    {
+        // TODO: remove


I meant to delete this; will get rid of it in the next revision.

SingleAccretion · 2023-12-08T09:06:51Z

src/coreclr/jit/fgdiagnostic.cpp

@@ -2957,6 +2957,13 @@ void Compiler::fgDebugCheckBBlist(bool checkBBNum /* = false */, bool checkBBRef

        maxBBNum = max(maxBBNum, block->bbNum);

+        // BBJ_COND's normal (false) jump target is expected to be the next block
+        // TODO: Allow bbNormalJumpDest to diverge from bbNext


A little tip for writing future TODOs: in ongoing work, it is handy to add a unique postfix to a TODO, e. g. TODO-NoFallThrough, s. t. they can be text-searched for easily afterwards.

Thank you for the tip; I realized this might be a better approach about halfway through... I'll update the TODOs I introduced to make them easier to Ctrl+F when I remove them in the next PR.

amanasifkhalid · 2023-12-08T12:09:03Z

CC @dotnet/jit-contrib, @AndyAyersMS PTAL -- sorry for the wait on this. No diffs except for TP impact.

BruceForstall · 2023-12-08T18:45:14Z

What is the terminology we want to use for the two BBJ_COND branches? With this PR we now have "bbNormalJumpDest" indicating the "false" path and "bbJumpDest" indicating the "true" path. Neither of these words seem satisfactory to me, especially "normal". It seems like we should have "bbFalseJumpDest" and "bbTrueJumpDest" for BBJ_COND. For cases with only a single destination (e.g., BBJ_ALWAYS), just "bbJumpDest" seems fine.

We over-emphasize the word "jump"; maybe we can remove it from all those places, instead having "bbFalseDest", "bbTrueDest", "bbDest", "bbSwtDest", "bbEhfDest", "bbKind", "BBKinds", etc.

And maybe "Dest" should instead be "Target": "bbFalseTarget", "bbTrueTarget", "bbTarget", "bbSwtTarget", "bbEhfTarget". I think this is my preferred option.

@dotnet/jit-contrib Comments?

SingleAccretion · 2023-12-08T18:49:39Z

FWIW, here's the origin of the "normal" terminology: #93772 (comment).

I agree it would be best to use "True/False", the problem was/is that we use GetJumpDest for the "true" path currently.

amanasifkhalid · 2023-12-08T18:59:17Z

I agree this would be a good time to rename these members.

It seems like we should have "bbFalseJumpDest" and "bbTrueJumpDest" for BBJ_COND.

For bbTrueTarget, are you suggesting we introduce a new BasicBlock* member to the union of jump targets that essentially functions as an alias for bbJumpDest, and then rename bbNormalJumpDest to bbFalseTarget? Or would you prefer a new jump kind altogether for BBJ_COND, like we do for BBJ_SWITCH? This new type would be a pointer to a struct of two BasicBlock* members: the true and the false targets. This would also come with new helper methods specific to BBJ_COND (like SetJumpKindAndTarget, etc.) for manipulating this new jump kind.

I concur with your other naming suggestions, @BruceForstall.

BruceForstall · 2023-12-08T19:23:43Z

For bbTrueTarget, are you suggesting we introduce a new BasicBlock* member to the union of jump targets that essentially functions as an alias for bbJumpDest, and then rename bbNormalJumpDest to bbFalseTarget?

Yes, this is what I was thinking.

Yes, there would be new helpers that would assert on BBJ_COND and return/set the new union member.

This probably means various "switch" cases where BBJ_COND behavior re-uses the same "branch" behavior and bbJumpDest access as other kinds would have to be split out, to access the new member. Maybe that's ok anyway, and clearer. (And the native compilers hopefully will "re-merge" if possible in the generated code.)

AndyAyersMS · 2023-12-08T19:57:00Z

I agree true/false makes more sense, as abstractly before block layout all control flow transfers are jumps.

Ultimately, I would like to see the blocks refer to FlowEdges instead of directly to blocks, so for BBJ_COND we'd have say TrueEdge and FalseEdge, and we'd enhance FlowEdge to refer to the target block (currently implicit); but we could also have combo methods that then access the edge and then its target. But if/when we get to this point we can still have BasicBlock* valued TrueTarget and FalseTarget methods on a block, they'll just do a bit more work.

BruceForstall · 2023-12-08T20:08:46Z

Ultimately, ...

I presume you're stating this as a long-term goal, and not something we should consider for this, or a short-term PR? Thus, we can evolve in that direction but still take name changes and block pointer implementation as proposed (if agreed to)?

amanasifkhalid · 2023-12-08T22:56:09Z

I've renamed the BasicBlock members @BruceForstall mentioned above, and added new helper methods for setting/accessing/checking BBJ_COND's true and false successors. I didn't add a BBJ_COND-specific variant of SetKindAndTarget in this change, as that might be best reserved for follow-up work once the false successor can diverge from bbNext, and we need a way to explicitly set both the true and false targets -- would we like a separate definition of SetKindAndTarget for initializing bbTrueTarget?

I apologize for the size of this change; at the very least, it does not produce any asmdiffs locally.

jakobbotsch · 2023-12-08T23:38:39Z

would we like a separate definition of SetKindAndTarget for initializing bbTrueTarget?

I think splitting these overloads to separate methods would make sense -- e.g. SetSwitch, SetCond and so on.

jakobbotsch · 2023-12-09T00:25:50Z

src/coreclr/jit/block.h

+        return (bbFalseTarget == target);
+    }
+
+    void SetCondKindAndTarget(BasicBlock* target)


This doesn't exactly roll off the tongue -- why not SetCond? Eventually we would hopefully end up with just SetCond(BasicBlock* falseTarget, BasicBlock* trueTarget), SetSwitch(BBswtDesc* swtTarget) and so on.

I can shorten the names of all the SetKind... variants. Would you be ok with SetKindAndTarget being left as-is? I'm not sure how we can intuitively shorten that one while still conveying it's setting the jump kind and target.

Yes, I think SetKindAndTarget makes sense -- it's sort of a low-level operation, while these other ones are more high-level.

AndyAyersMS

A few small suggestions. Feel free to defer those to a subsequent PR.

AndyAyersMS · 2023-12-11T15:43:06Z

src/coreclr/jit/block.h

    }

-    BBswtDesc* GetJumpSwt() const
+    BBswtDesc* GetSwitchTarget() const


Nit: switches have multiple targets, so maybe?

Suggested change

BBswtDesc* GetSwitchTarget() const

BBswtDesc* GetSwitchTargets() const

AndyAyersMS · 2023-12-11T15:43:26Z

src/coreclr/jit/block.h

    }

-    BBehfDesc* GetJumpEhf() const
+    BBehfDesc* GetEhfTarget() const


Similar nit

Suggested change

BBehfDesc* GetEhfTarget() const

BBehfDesc* GetEhfTargets() const

AndyAyersMS · 2023-12-11T15:44:18Z

src/coreclr/jit/block.h

-        assert(HasInitializedJumpDest());
-        return (bbJumpDest == jumpDest);
+        // BBJ_COND should use TrueTargetIs
+        assert(!KindIs(BBJ_COND));


Seems like you should also assert for BBJ_SWITCH and BBJ_EHFINALLYRET in these (here and elsewhere).

I think we're already implicitly asserting for BBJ_SWITCH and BBJ_EHFINALLYRET by asserting HasInitializedTarget, as that asserts HasTarget, which checks KindIs(BBJ_ALWAYS, BBJ_CALLFINALLY, BBJ_COND, BBJ_EHCATCHRET, BBJ_EHFILTERRET, BBJ_LEAVE). But that's a lot of indirection, so I can explicitly assert for BBJ_SWITCH and BBJ_EHFINALLYRET, too.

AndyAyersMS · 2023-12-11T15:44:55Z

src/coreclr/jit/block.h

    }

-    void SetJumpEhf(BBehfDesc* jumpEhf)
+    void SetEhfTarget(BBehfDesc* ehfTarget)


Suggested change

void SetEhfTarget(BBehfDesc* ehfTarget)

void SetEhfTargets(BBehfDesc* ehfTargets)

AndyAyersMS · 2023-12-11T15:45:23Z

src/coreclr/jit/block.h

+        unsigned    bbTargetOffs; // PC offset (temporary only)
+        BasicBlock* bbTarget;     // basic block
+        BasicBlock* bbTrueTarget; // BBJ_COND jump target when its condition is true (alias for bbTarget)
+        BBswtDesc*  bbSwtTarget;  // switch descriptor


Suggested change

BBswtDesc* bbSwtTarget; // switch descriptor

BBswtDesc* bbSwtTargets; // switch descriptor

AndyAyersMS · 2023-12-11T15:45:36Z

src/coreclr/jit/block.h

+        BasicBlock* bbTarget;     // basic block
+        BasicBlock* bbTrueTarget; // BBJ_COND jump target when its condition is true (alias for bbTarget)
+        BBswtDesc*  bbSwtTarget;  // switch descriptor
+        BBehfDesc*  bbEhfTarget;  // BBJ_EHFINALLYRET descriptor


Suggested change

BBehfDesc* bbEhfTarget; // BBJ_EHFINALLYRET descriptor

BBehfDesc* bbEhfTargets; // BBJ_EHFINALLYRET descriptor

AndyAyersMS · 2023-12-11T15:46:20Z

src/coreclr/jit/block.h

+    BasicBlock* GetTarget() const
+    {
+        // BBJ_COND should use GetTrueTarget
+        assert(!KindIs(BBJ_COND));


Seems like you should also assert for BBJ_SWITCH and BBJ_EHFINALLYRET in these (here and elsewhere).

AndyAyersMS · 2023-12-11T15:46:28Z

src/coreclr/jit/block.h

+    void SetTarget(BasicBlock* target)
+    {
+        // BBJ_COND should use SetTrueTarget
+        assert(!KindIs(BBJ_COND));


Seems like you should also assert for BBJ_SWITCH and BBJ_EHFINALLYRET in these (here and elsewhere).

AndyAyersMS · 2023-12-11T15:55:18Z

src/coreclr/jit/codegenlinear.cpp

                // `block` (inclusive). Thus, we need to ensure there is a label on the lexical fall-through
                // block, even if one is not otherwise needed, to be able to calculate the size of this
                // loop (loop size is calculated by walking the instruction groups; see emitter::getLoopSize()).

-                if (block->GetJumpDest()->isLoopAlign())
+                if (block->GetTarget()->isLoopAlign())


This kind of code duplication is a bit unfortunate. Obviously early on there is a distinction for BBJ_COND but later on there isn't...

One half-baked idea is to have some variant of BBJ_COND that only appears late, once we allow fall throughs, and can't appear earlier. But let's just leave this as is for now.

amanasifkhalid · 2023-12-11T17:00:00Z

@AndyAyersMS thank you for the review; I've applied your feedback. The new asserts you suggested triggered in Compiler::fgSplitBlockAtEnd, as we were previously calling GetTarget() on a BBJ_EHFINALLYRET block, and then creating a new BBJ_EHFINALLYRET with BasicBlock::New(Compiler* compiler, BBKinds kind, BasicBlock* target). I've refactored fgSplitBlockAtEnd by creating a new variant of BasicBlock::New specifically for BBJ_EHFINALLYRET (kind of like how we have a variant for BBJ_SWITCH), and moving the logic for creating new blocks in fgSplitBlockAtEnd into a switch statement. It's slightly more code duplication, but almost all of the logic after the block creation is generalized across block types in that method.

amanasifkhalid · 2023-12-11T19:47:58Z

OSX arm64 NativeAOT failure looks unrelated.

jakobbotsch · 2023-12-12T20:26:37Z

I hit asserts with JitDump enabled after this change in some simple code examples. For example

[MethodImpl(MethodImplOptions.NoInlining)]
static int abs(int num) => num < 0 ? -num : num;

hits

Assert failure(PID 21588 [0x00005454], Thread: 51020 [0xc74c]): Assertion failed 'KindIs(BBJ_COND)' in 'Program:abs(int):int' during 'If conversion' (IL size 9; hash 0x993542c0; FullOpts)

    File: C:\dev\dotnet\runtime4\src\coreclr\jit\block.h Line: 661
    Image: D:\dev\core_roots\6c7e6e2e50550737260e40f0b472158bc15a6f82\corerun.exe

when DOTNET_JitDump=abs. Can you take a look? (Also might be a good idea to run SPMI through with JitDump=*).

amanasifkhalid · 2023-12-12T20:28:21Z

@jakobbotsch Thank you for letting me know, taking a look now...

…#95934) In #95773, I incorrectly assumed m_startBlock would always be a BBJ_COND in OptIfConversionDsc::IfConvertDump, but it can be converted to a BBJ_ALWAYS before being dumped, thus hitting an assert when m_startBlock->GetTrueTarget() is called. This is fixed by calling the correct target accessor method depending on the type of m_startBlock. (Note that IfConvertDump is only called if dumps are enabled, hence why this assert wasn't initially hit in CI.)

amanasifkhalid added 4 commits December 7, 2023 14:17

Introduce bbNormalJumpDest

7075361

Replace Next() with GetNormalJumpDest()

edcd975

Replace NextIs() with HasNormalJumpTo()

bdc886d

Style

4c71330

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 7, 2023

ghost assigned amanasifkhalid Dec 7, 2023

amanasifkhalid mentioned this pull request Dec 7, 2023

JIT: Add explicit block fallthrough successor #93772

Closed

amanasifkhalid marked this pull request as ready for review December 8, 2023 01:04

amanasifkhalid commented Dec 8, 2023

View reviewed changes

SingleAccretion reviewed Dec 8, 2023

View reviewed changes

amanasifkhalid added 3 commits December 8, 2023 10:56

Update TODO comments

bdcf9ea

Style

4e94533

Add comment explaining assert

3855094

amanasifkhalid added 2 commits December 8, 2023 14:51

Rename bbNormalJumpDest to bbFalseTarget

f630b5d

Rename bbJumpDest to bbTarget

5be6ed2

amanasifkhalid added 2 commits December 8, 2023 15:00

Rename SetJumpKindAndTarget variants

a286d86

Rename bbJumpKind

9ff966b

amanasifkhalid added 5 commits December 8, 2023 15:10

Rename bbJumpOffs

e632217

Rename bbJumpSwt

dcf7ce9

Rename bbJumpEhf

07ff2dd

Rename HasJumpTo

e2ab378

Rename method args, dspJumpKind

92139e3

amanasifkhalid added 3 commits December 8, 2023 17:41

Style

69cbbc7

Fix comment

a35bd40

Fix bbNext/bbTarget access for BBJ_COND in BBSuccList

f45a824

amanasifkhalid added 2 commits December 8, 2023 19:04

Split out SetSwtKindAndTarget and SetEhfKindAndTarget

2e5fc28

Add SetCondKindAndTarget

a8f3ffd

jakobbotsch reviewed Dec 9, 2023

View reviewed changes

amanasifkhalid added 4 commits December 8, 2023 19:36

Rename to SetCond

b1de3c6

Rename to SetSwitch

a038073

Rename to SetEhf

3625c39

Rename to GetSwitchTarget (for consistency)

fbd3943

AndyAyersMS approved these changes Dec 11, 2023

View reviewed changes

amanasifkhalid added 5 commits December 11, 2023 11:20

Rename GetSwitchTarget > GetSwitchTargets

ed0b149

Rename GetEhfTarget > GetEhfTargets

7f1fbb7

Rename bbSwtTarget > bbSwtTargets

e453580

Rename bbEhfTarget/SetEhfTarget

c78e766

Add asserts

f445e29

BruceForstall approved these changes Dec 11, 2023

View reviewed changes

amanasifkhalid merged commit 6c7e6e2 into dotnet:main Dec 11, 2023
124 of 129 checks passed

amanasifkhalid mentioned this pull request Dec 11, 2023

JIT: Port loop alignment to new loop representation #95836

Merged

amanasifkhalid mentioned this pull request Dec 12, 2023

JIT: Fix BBJ_COND-related assert in OptIfConversionDsc::IfConvertDump #95934

Merged

This was referenced Jan 5, 2024

[RISC-V] Enable On Stack Replacement #96558

Merged

Replace call to GetFalseTarget method with GetTarget inside fgRemoveConditionalJump #96560

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JIT: Add explicit successor for BBJ_COND false branch #95773

JIT: Add explicit successor for BBJ_COND false branch #95773

amanasifkhalid commented Dec 7, 2023

ghost commented Dec 7, 2023

amanasifkhalid Dec 8, 2023

SingleAccretion Dec 8, 2023

amanasifkhalid Dec 8, 2023

amanasifkhalid commented Dec 8, 2023

BruceForstall commented Dec 8, 2023

SingleAccretion commented Dec 8, 2023

amanasifkhalid commented Dec 8, 2023

BruceForstall commented Dec 8, 2023

AndyAyersMS commented Dec 8, 2023

BruceForstall commented Dec 8, 2023

amanasifkhalid commented Dec 8, 2023

jakobbotsch commented Dec 8, 2023

jakobbotsch Dec 9, 2023

amanasifkhalid Dec 9, 2023

jakobbotsch Dec 9, 2023

AndyAyersMS left a comment

AndyAyersMS Dec 11, 2023

AndyAyersMS Dec 11, 2023

AndyAyersMS Dec 11, 2023

amanasifkhalid Dec 11, 2023

AndyAyersMS Dec 11, 2023

AndyAyersMS Dec 11, 2023

AndyAyersMS Dec 11, 2023

AndyAyersMS Dec 11, 2023

AndyAyersMS Dec 11, 2023

AndyAyersMS Dec 11, 2023

amanasifkhalid commented Dec 11, 2023

amanasifkhalid commented Dec 11, 2023

jakobbotsch commented Dec 12, 2023

amanasifkhalid commented Dec 12, 2023

	BBswtDesc* GetSwitchTarget() const
	BBswtDesc* GetSwitchTargets() const

	BBehfDesc* GetEhfTarget() const
	BBehfDesc* GetEhfTargets() const

	void SetEhfTarget(BBehfDesc* ehfTarget)
	void SetEhfTargets(BBehfDesc* ehfTargets)

	BBswtDesc* bbSwtTarget; // switch descriptor
	BBswtDesc* bbSwtTargets; // switch descriptor

	BBehfDesc* bbEhfTarget; // BBJ_EHFINALLYRET descriptor
	BBehfDesc* bbEhfTargets; // BBJ_EHFINALLYRET descriptor

JIT: Add explicit successor for BBJ_COND false branch #95773

JIT: Add explicit successor for BBJ_COND false branch #95773

Conversation

amanasifkhalid commented Dec 7, 2023

ghost commented Dec 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanasifkhalid commented Dec 8, 2023

BruceForstall commented Dec 8, 2023

SingleAccretion commented Dec 8, 2023

amanasifkhalid commented Dec 8, 2023

BruceForstall commented Dec 8, 2023

AndyAyersMS commented Dec 8, 2023

BruceForstall commented Dec 8, 2023

amanasifkhalid commented Dec 8, 2023

jakobbotsch commented Dec 8, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndyAyersMS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amanasifkhalid commented Dec 11, 2023

amanasifkhalid commented Dec 11, 2023

jakobbotsch commented Dec 12, 2023

amanasifkhalid commented Dec 12, 2023