JIT: Make BasicBlock::bbPrev and bbNext private by amanasifkhalid · Pull Request #93032 · dotnet/runtime

amanasifkhalid · 2023-10-04T21:25:58Z

Followup to #92908, and next step for #93020. CC @dotnet/jit-contrib.

… bbj-none

ghost · 2023-10-04T21:26:15Z

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

Issue Details

Followup to #92908, and next step for #93020. CC @dotnet/jit-contrib.

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

BruceForstall · 2023-10-05T05:37:41Z

No asm diffs but some TP diffs (maybe because SetBBPrev() does more than what it replaces?)

BruceForstall · 2023-10-05T05:45:16Z

Seems like we should work to avoid too many GetBBNext() calls in the code (maybe as a follow-up?)

E.g., instead of (block->bbNext == nullptr) => a new function block->IsLast(), and block->bbPrev == nullptr => block->IsFirst().

block->GetBBNext() == compiler->fgFirstColdBlock => block->IsLastHotBlock(compiler) (true also for no cold section and block->IsEnd()?) Similarly, block->IsFirstColdBlock(compiler)?

This might be more controversial, but block->GetBBNext() == block2 => block->NextIs(block2)? Similarly, block->GetBBPrev() == block2 => block->PrevIs(block2).

Given the above, should we name GetBBNext() simply Next() and GetBBPrev() simply Prev()? The set functions could be SetNext() and SetPrev().

Looks like there are a few more places that can use the block iterators instead of manual for loops (though maybe not? the iterators don't tolerate block list modifications)

SingleAccretion · 2023-10-05T13:07:12Z

src/coreclr/jit/block.h

+        }
+    }
+
+    BasicBlock* GetBBNext() const


While we're renaming things, we could give these more "obvious" names, e. g. GetNextBlock.

amanasifkhalid · 2023-10-05T14:50:54Z

No asm diffs but some TP diffs (maybe because SetBBPrev() does more than what it replaces?)

We previously had BasicBlock::setNext() that updated bbNext, and also null-checked the new next block to see if it could update its bbPrev; most of the time, we'd just directly update bbNext, and would sparingly call setNext() if we also intended to update the next block's bbPrev. Would it make sense to not have SetBBNext() do this additional work, and keep setNext() around to do it? I think this would reduce/eliminate the TP diffs, but we'd lose the guarantee that bbNext and bbNext->bbPrev are consistent within SetBBNext().

Seems like we should work to avoid too many GetBBNext() calls in the code (maybe as a follow-up?)

I'm happy to add helper methods for checking bbNext/bbPrev. This is for readability, since GetBBNext() should be getting inlined, right?

amanasifkhalid · 2023-10-05T15:16:13Z

E.g., instead of (block->bbNext == nullptr) => a new function block->IsLast(), and block->bbPrev == nullptr => block->IsFirst().

Would it be preferable to check against fgFirstBB and fgLastBB instead of adding new methods? Ditto checking if block == fgFirstColdBlock instead of a new method (though for checking if a block is the last hot block, I agree we'd need a new method to avoid calling GetBBNext()).

BruceForstall · 2023-10-05T15:51:49Z

Would it make sense to not have SetBBNext() do this additional work, and keep setNext() around to do it?

No, it's probably best to leave it as you have it now.

I'm happy to add helper methods for checking bbNext/bbPrev. This is for readability, since GetBBNext() should be getting inlined, right?

Yes.

Would it be preferable to check against fgFirstBB and fgLastBB instead of adding new methods? Ditto checking if block == fgFirstColdBlock instead of a new method (though for checking if a block is the last hot block, I agree we'd need a new method to avoid calling GetBBNext()).

We don't always have a Compiler* available that can be used to get fgFirstBB/fgLastBB.

For fgFirstColdBlock we have no other way to check except using that variable, which then requires a Compiler*.

AndyAyersMS · 2023-10-05T16:37:54Z

I suspect over time we're going to see more and more cases where BB operations are contextual and need access to the compiler instance. At various times I've wondered if blocks should just point back to some parent object (either the compiler or some sort of "flow graph" object, but that seems wasteful. Given that blocks are all the same size we could perhaps play tricks to factor out common fields, if we say allocated them contiguously. For now, I think providing the context when needed via args is ok.

One issue with either a context arg or a parent object is that when we inline we do some funny things as there are two compiler objects around, and so it's possible to consult the wrong one....

amanasifkhalid · 2023-10-05T20:44:04Z

@BruceForstall @AndyAyersMS PTAL. CI is clean, but small TP diffs. I'm planning on making the jump target members private next; if we can assert the jump kind is valid every time we try to read the jump destination, then maybe we can remove code that sets the jump destination to null when changing the jump kind. This might make up for the TP regression in Release builds.

AndyAyersMS

This LGTM. Thanks!

BruceForstall

Found a few nits that could be updated. Can't say I looked at every case.

BruceForstall · 2023-10-05T22:17:07Z

src/coreclr/jit/block.cpp

 }

+//------------------------------------------------------------------------
+// IsLastHotBlock: see if this is the last block before the cold section


Should we have a corresponding IsFirstColdBlock(Compiler*)?

I can add that in for consistency.

BruceForstall · 2023-10-05T22:18:01Z

src/coreclr/jit/block.h

+        assert((m_block->IsLast()) || m_block->Next()->PrevIs(m_block));
+        assert((m_block->IsFirst()) || m_block->Prev()->NextIs(m_block));


With the function, we can remove an unnecessary set of parens

Suggested change

assert((m_block->IsLast()) || m_block->Next()->PrevIs(m_block));

assert((m_block->IsFirst()) || m_block->Prev()->NextIs(m_block));

assert(m_block->IsLast() || m_block->Next()->PrevIs(m_block));

assert(m_block->IsFirst() || m_block->Prev()->NextIs(m_block));

Thanks for catching all these nits (sorry, these kinds of PRs seem especially tedious to review).

BruceForstall · 2023-10-05T22:20:48Z

src/coreclr/jit/codegencommon.cpp

    assert(block != nullptr);
    const VARSET_TP& gcrefVarsArg(GetEmitter()->emitThisGCrefVars);
-    bool             last = (block->bbNext == nullptr);
+    bool             last = (block->IsLast());


Suggested change

bool last = (block->IsLast());

bool last = block->IsLast();

or just substitute it in the call below and eliminate the last var

BruceForstall · 2023-10-05T22:20:57Z

src/coreclr/jit/codegencommon.cpp

    JITDUMP("Reserving funclet epilog IG for block " FMT_BB "\n", block->bbNum);

-    bool last = (block->bbNext == nullptr);
+    bool last = (block->IsLast());


BruceForstall · 2023-10-05T22:22:03Z

src/coreclr/jit/codegenlinear.cpp

        /* Is this the last block, and are there any open scopes left ? */

-        bool isLastBlockProcessed = (block->bbNext == nullptr);
+        bool isLastBlockProcessed = (block->IsLast());


Suggested change

bool isLastBlockProcessed = (block->IsLast());

bool isLastBlockProcessed = block->IsLast();

BruceForstall · 2023-10-05T22:22:13Z

src/coreclr/jit/codegenlinear.cpp

        if (block->isBBCallAlwaysPair())
        {
-            isLastBlockProcessed = (block->bbNext->bbNext == nullptr);
+            isLastBlockProcessed = (block->Next()->IsLast());


Suggested change

isLastBlockProcessed = (block->Next()->IsLast());

isLastBlockProcessed = block->Next()->IsLast();

BruceForstall · 2023-10-05T22:22:26Z

src/coreclr/jit/codegenlinear.cpp

            // if the next block is a BBJ_RETURN, an epilog will be generated, but there may be some instructions
            // generated before the OS epilog starts, such as a GS cookie check.
-            if ((block->bbNext == nullptr) || !BasicBlock::sameEHRegion(block, block->bbNext))
+            if ((block->IsLast()) || !BasicBlock::sameEHRegion(block, block->Next()))


Suggested change

if ((block->IsLast()) || !BasicBlock::sameEHRegion(block, block->Next()))

if (block->IsLast() || !BasicBlock::sameEHRegion(block, block->Next()))

BruceForstall · 2023-10-05T22:23:13Z

src/coreclr/jit/codegenlinear.cpp

        }

-        if ((block->bbNext != nullptr) && (block->bbNext->isLoopAlign()))
+        if (!block->IsLast() && (block->Next()->isLoopAlign()))


Suggested change

if (!block->IsLast() && (block->Next()->isLoopAlign()))

if (!block->IsLast() && block->Next()->isLoopAlign())

BruceForstall · 2023-10-05T22:24:03Z

src/coreclr/jit/compiler.cpp

        }

-        if ((block->bbNext != nullptr) && (block->bbNext->isLoopAlign()))
+        if (!block->IsLast() && (block->Next()->isLoopAlign()))


Suggested change

if (!block->IsLast() && (block->Next()->isLoopAlign()))

if (!block->IsLast() && block->Next()->isLoopAlign())

BruceForstall · 2023-10-05T22:28:50Z

src/coreclr/jit/fgbasic.cpp

        printf("Relocated block%s [" FMT_BB ".." FMT_BB "] inserted after " FMT_BB "%s\n", (bStart == bEnd) ? "" : "s",
               bStart->bbNum, bEnd->bbNum, insertAfterBlk->bbNum,
-               (insertAfterBlk->bbNext == nullptr) ? " at the end of method" : "");
+               (insertAfterBlk->IsLast()) ? " at the end of method" : "");


Suggested change

(insertAfterBlk->IsLast()) ? " at the end of method" : "");

insertAfterBlk->IsLast() ? " at the end of method" : "");

amanasifkhalid · 2023-10-05T23:26:19Z

@BruceForstall I think I fixed all the style issues introduced (plus a couple extra I found along the way).

BruceForstall

LGTM. Thanks!

amanasifkhalid added 7 commits October 2, 2023 17:29

Make BasicBlock::bbJumpKind private

3a4bdf5

Convert bbJumpKind comparisons to KindIs()

2378797

Style

a8c8a6c

Convert case

7aadbdc

Typo

754743d

Merge branch 'main' of https://github.com/amanasifkhalid/runtime into…

b31fee9

… bbj-none

Make bbPrev and bbNext private

0a16a8f

ghost assigned amanasifkhalid Oct 4, 2023

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

SingleAccretion reviewed Oct 5, 2023

View reviewed changes

amanasifkhalid added 2 commits October 5, 2023 12:10

Add IsFirst() and IsLast()

254c362

Add BasicBlock::IsLastHotBlock()

f6dfb21

amanasifkhalid added 3 commits October 5, 2023 13:30

Add NextIs() and PrevIs()

52370b0

Rename GetBBNext() et al

b0fc610

Style

59e9455

build-analysis bot mentioned this pull request Oct 5, 2023

Test infra failure on iOS.Device.Aot.Test.WorkItemExecution CI leg #92792

Closed

3 tasks

AndyAyersMS approved these changes Oct 5, 2023

View reviewed changes

BruceForstall reviewed Oct 5, 2023

View reviewed changes

amanasifkhalid added 3 commits October 5, 2023 18:50

Add IsFirstColdBlock

1fb10a8

Address feedback

5bb73fe

Missed one

c03b7c9

BruceForstall approved these changes Oct 5, 2023

View reviewed changes

amanasifkhalid merged commit 22d034f into dotnet:main Oct 6, 2023

AndyAyersMS mentioned this pull request Oct 6, 2023

JIT: Flow Graph Modernization and Improved Block Layout #93020

Closed

51 tasks

ghost locked as resolved and limited conversation to collaborators Nov 5, 2023

		assert((m_block->IsLast()) \|\| m_block->Next()->PrevIs(m_block));
		assert((m_block->IsFirst()) \|\| m_block->Prev()->NextIs(m_block));

	bool isLastBlockProcessed = (block->IsLast());
	bool isLastBlockProcessed = block->IsLast();

	isLastBlockProcessed = (block->Next()->IsLast());
	isLastBlockProcessed = block->Next()->IsLast();

	if ((block->IsLast()) \|\| !BasicBlock::sameEHRegion(block, block->Next()))
	if (block->IsLast() \|\| !BasicBlock::sameEHRegion(block, block->Next()))

	if (!block->IsLast() && (block->Next()->isLoopAlign()))
	if (!block->IsLast() && block->Next()->isLoopAlign())

	(insertAfterBlk->IsLast()) ? " at the end of method" : "");
	insertAfterBlk->IsLast() ? " at the end of method" : "");

Conversation

amanasifkhalid commented Oct 4, 2023

Uh oh!

ghost commented Oct 4, 2023

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amanasifkhalid commented Oct 5, 2023

Uh oh!

amanasifkhalid commented Oct 5, 2023

Uh oh!

BruceForstall commented Oct 5, 2023

Uh oh!

AndyAyersMS commented Oct 5, 2023

Uh oh!

amanasifkhalid commented Oct 5, 2023

Uh oh!

AndyAyersMS left a comment

Choose a reason for hiding this comment

Uh oh!

BruceForstall left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amanasifkhalid commented Oct 5, 2023

Uh oh!

BruceForstall left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

BruceForstall commented Oct 5, 2023 •

edited

Loading

BruceForstall commented Oct 5, 2023 •

edited

Loading