Filter blocks to be used for hiding alignment instruction #62262

kunalspathak · 2021-12-02T01:52:51Z

Using bbNaturalLoopNum track which blocks are in a loop and do not use those blocks for hiding align instruction.
Also do not use blocks that are part of callf/finally pair for hiding align instruction.

This reverts commit 1fc26a5.

ghost · 2021-12-02T01:52:58Z

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

Issue Details

Using bbNaturalLoopNum track which blocks are in a loop and do not use those blocks for hiding align instruction.
Also do not use blocks that are part of callf/finally pair for hiding align instruction.

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

kunalspathak · 2021-12-02T16:27:19Z

libraries-jitstress failures are related to Assertion failed '(op3Reg != targetReg) || (op1Reg == targetReg)' during 'Generate code' #62267 , Assertion failed 'child->isContained()' #60330 and System.Net.Security.Tests failed with System.IO.IOException #52362
jitstress failures are related to Test failure JIT\\Directed\\LoopAlignment\\LoopsToProcess\\LoopsToProcess.cmd #62260

kunalspathak · 2021-12-02T16:48:13Z

@dotnet/jit-contrib

src/coreclr/jit/compiler.cpp

BruceForstall · 2021-12-02T17:51:49Z

src/coreclr/jit/compiler.cpp

-                bbHavingAlign = block;
-                JITDUMP(FMT_BB ", bbWeight=" FMT_WT " ends with unconditional 'jmp' \n", block->bbNum, block->bbWeight);
+                // Only if they are not part of the loop that will be aligned
+                if ((block->bbNatLoopNum == BasicBlock::NOT_IN_LOOP) || (block->bbNatLoopNum != currentLoopNum))


Suggested change

if ((block->bbNatLoopNum == BasicBlock::NOT_IN_LOOP) || (block->bbNatLoopNum != currentLoopNum))

if (currentLoopNum != BasicBlock::NOT_IN_LOOP)

We only care whether we're in an aligned loop or not.

Assuming I will keep the reset of currentLoopNum = NOT_IN_LOOP inside bbJumpDest check (the way I have currently), this will not work. currentLoopNum is initialized with NOT_IN_LOOP and we will never select a block that ends with jmp until we come across a loop that sets a valid value of currentLoopNum.

Right, my logic is backwards. It should be:

if (currentLoopNum == BasicBlock::NOT_IN_LOOP) // ... ok to insert alignment insruction behind JMP

The idea being:

If curLoopNum == NOT_IN_LOOP, then we aren't processing an aligned loop. In this case, we can put an align instruction behind a JMP.

If curLoopNum != NOT_IN_LOOP, then block is within a loop that we are aligning. Don't put align after a JMP instruction; wait until we reach the end of the aligned loop (determined by comparing curLoopNum against bbNatLoopNum)

BruceForstall · 2021-12-02T17:58:35Z

src/coreclr/jit/compiler.cpp

+
+        if ((block->bbJumpDest != nullptr) && (block->bbJumpDest->isLoopAlign()))
+        {
+            // This is the last block of current loop. Reset the latestLoopNum so we can resume
+            // placing the align behind jmp for subsequent loops.
+            currentLoopNum = BasicBlock::NOT_IN_LOOP;
+        }


I suggest removing this code and adding the following at the top of the loop:

if (currentLoopNum != BasicBlock::NOT_IN_LOOP) { // We've been processing blocks within an aligned loop. Are we out of that loop now? if (currentLoopNum != block->bbNatLoopNum) { currentLoopNum = BasicBlock::NOT_IN_LOOP; } }

Note that this only works because we only align innermost loops, since the bbNatLoopNum has the innermost loop number for a block.

Then, if this block is a BBJ_ALWAYS, subsequent code will allow it to be used to add alignment. And, if the next block starts a loop, the code at the end will set up currentLoopNum for the next iteration.

The current implementation will make sure we reset the currentLoopNum for the earliest back-edge we see. With your suggestion, if we have multiple back-edges to the loop, and in between, one of the block ended with jmp, we won't be able to use that for hiding the alignment because the currentLoopNum will still match the block->bbNatLoopNum.

That's correct, but do you think that's a limitation? After all, if the bbNatLoopNum still matches, it's still part of the same inner loop. Is this concern because the first back-edge is considered the loop end in emitter::getLoopSize()? I would argue that is incorrect, even if it's not an unreasonable simplification. I don't think this situation arises often anyway, as most loops have a single back edge to the top.

Is this concern because the first back-edge is considered the loop end in emitter::getLoopSize()?

Yes. My thinking behind the existing logic is that if getLoopSize() only considers the first back-edge, then we should open the opportunity to find jmp after that point and use them if needed instead of waiting to complete the loop.

I would argue, alternatively, that we should think of ways to fix getLoopSize() to respect the loop package's determination about what constitutes the inner loop.

Note that, even as is, there are a few other concerns: (1) bbJumpDest is part of a union and not valid for all jump types, (2) you don't check that the branch is to the beginning of the loop we care about; it could be a forward branch so a subsequent loop that is aligned.

I would argue, alternatively, that we should think of ways to fix getLoopSize() to respect the loop package's determination about what constitutes the inner loop.

I think that needs separate discussion and should be not part of this PR. The initial decision was to just check the earliest back-edge to determine the loop size. If we want something smarter, we can definitely look at the weights of other blocks (the ones that follows after the earlier back-edge) to determine if those blocks are hot enough and do we need to include them as part of our loop size. It might further complicate things and we don't even have data to check if it will give us benefit.

(1) bbJumpDest is part of a union and not valid for all jump types,

That's true. We check that in codegenlinear.cpp and I should add similar check here as well.

(2) you don't check that the branch is to the beginning of the loop we care about; it could be a forward branch so a subsequent loop that is aligned

To make that work properly, we will need block renumbering, after which I can check it based on bbNum if it is forward or backward, right?

True, no need to change getLoopSize() here. I'm arguing that this code code be the "correct" general case, and getLoopSize can follow later, if necessary.

To make that work properly, we will need block renumbering, after which I can check it based on bbNum if it is forward or backward, right?

You could save away the block with the ALIGN bit and compare against that when looking for back edges.

However, I think my suggestion is simpler and doesn't require that.

BruceForstall

LGTM

kunalspathak · 2021-12-03T23:54:11Z

/azp run runtime-coreclr jitstress

kunalspathak · 2021-12-03T23:54:15Z

/azp run runtime-coreclr libraries-jitstress

azure-pipelines · 2021-12-03T23:54:26Z

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines · 2021-12-03T23:54:29Z

Azure Pipelines successfully started running 1 pipeline(s).

kunalspathak · 2021-12-06T14:46:48Z

Failures are #62285 and #62267

kunalspathak added 5 commits December 1, 2021 10:25

fix for dotnet#61899

1fc26a5

proper fix

99437fd

Fix for dotnet#62238

8d1203c

misc change

bb94ba6

Revert "fix for dotnet#61899"

75a9677

This reverts commit 1fc26a5.

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 2, 2021

fix formatting

10cd236

kunalspathak marked this pull request as ready for review December 2, 2021 16:48

fix formatting once again

9bafa99

BruceForstall requested changes Dec 2, 2021

View reviewed changes

ghost added needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels Dec 2, 2021

kunalspathak added 2 commits December 2, 2021 15:06

add validJumpKind check

af7d3b2

review comments

d2bc4bf

BruceForstall approved these changes Dec 3, 2021

View reviewed changes

kunalspathak merged commit a9947a1 into dotnet:main Dec 6, 2021

kunalspathak mentioned this pull request Dec 6, 2021

Test failure Interop\\PInvoke\\Vector2_3_4\\Vector2_3_4\\Vector2_3_4.cmd #61579

Closed

runfoapp bot mentioned this pull request Dec 6, 2021

jit/intrinsics/createspan_il/createspan_il.sh #62285

Closed

kunalspathak mentioned this pull request Dec 11, 2021

Test failure JIT/Performance/CodeQuality/Benchstones/BenchF/MatInv4/MatInv4/MatInv4.sh #61899

Closed

ghost locked as resolved and limited conversation to collaborators Jan 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filter blocks to be used for hiding alignment instruction #62262

Filter blocks to be used for hiding alignment instruction #62262

kunalspathak commented Dec 2, 2021 •

edited

Loading

ghost commented Dec 2, 2021

kunalspathak commented Dec 2, 2021

kunalspathak commented Dec 2, 2021

BruceForstall Dec 2, 2021

kunalspathak Dec 2, 2021

BruceForstall Dec 2, 2021

BruceForstall Dec 2, 2021

kunalspathak Dec 2, 2021

BruceForstall Dec 2, 2021

kunalspathak Dec 2, 2021 •

edited

Loading

BruceForstall Dec 2, 2021

kunalspathak Dec 2, 2021 •

edited

Loading

BruceForstall Dec 2, 2021

BruceForstall left a comment

kunalspathak commented Dec 3, 2021

kunalspathak commented Dec 3, 2021

azure-pipelines bot commented Dec 3, 2021

azure-pipelines bot commented Dec 3, 2021

kunalspathak commented Dec 6, 2021

	if ((block->bbNatLoopNum == BasicBlock::NOT_IN_LOOP) \|\| (block->bbNatLoopNum != currentLoopNum))
	if (currentLoopNum != BasicBlock::NOT_IN_LOOP)

Filter blocks to be used for hiding alignment instruction #62262

Filter blocks to be used for hiding alignment instruction #62262

Conversation

kunalspathak commented Dec 2, 2021 • edited Loading

ghost commented Dec 2, 2021

kunalspathak commented Dec 2, 2021

kunalspathak commented Dec 2, 2021

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

kunalspathak Dec 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kunalspathak Dec 2, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BruceForstall left a comment

Choose a reason for hiding this comment

kunalspathak commented Dec 3, 2021

kunalspathak commented Dec 3, 2021

azure-pipelines bot commented Dec 3, 2021

azure-pipelines bot commented Dec 3, 2021

kunalspathak commented Dec 6, 2021

kunalspathak commented Dec 2, 2021 •

edited

Loading

kunalspathak Dec 2, 2021 •

edited

Loading

kunalspathak Dec 2, 2021 •

edited

Loading