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.

Sign up for GitHub

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

Jump to bottom

JIT: Allow jump-to-next-block removal for blocks with alignment #97011

Merged
merged 3 commits into from
Feb 2, 2024
Merged

JIT: Allow jump-to-next-block removal for blocks with alignment #97011

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f47aaf8
Allow jump removal for aligned BBJ_ALWAYS
amanasifkhalid Jan 15, 2024
188f2b2
no merge
amanasifkhalid Jan 17, 2024
b1a138f
Revert "no merge"
amanasifkhalid Feb 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/coreclr/jit/block.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,7 @@ bool BasicBlock::IsFirstColdBlock(Compiler* compiler) const
bool BasicBlock::CanRemoveJumpToNext(Compiler* compiler) const
{
assert(KindIs(BBJ_ALWAYS));
return JumpsToNext() && (bbNext != compiler->fgFirstColdBlock);
return JumpsToNext() && (bbNext != compiler->fgFirstColdBlock) && (!hasAlign() || HasFlag(BBF_JMP_TO_NESTED_LOOP));
}

//------------------------------------------------------------------------
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/block.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -429,6 +429,7 @@ enum BasicBlockFlags : unsigned __int64
// (TODO: Remove this quirk after refactoring Compiler::fgFindInsertPoint)
BBF_OLD_LOOP_HEADER_QUIRK = MAKE_BBFLAG(42), // Block was the header ('entry') of a loop recognized by old loop finding
BBF_HAS_VALUE_PROFILE = MAKE_BBFLAG(43), // Block has a node that needs a value probing
BBF_JMP_TO_NESTED_LOOP = MAKE_BBFLAG(44),
Copy link
Member

Choose a reason for hiding this comment

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

Please add a comment

Copy link
Member Author

@amanasifkhalid amanasifkhalid Jan 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I plan to remove this flag before merging; this was just a quick-and-dirty way for determining when to remove a jump in a BBJ_ALWAYS block with alignment (to get the diffs Kunal wanted here).


// The following are sets of flags.

Expand Down
7 changes: 7 additions & 0 deletions src/coreclr/jit/compiler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5431,6 +5431,13 @@ PhaseStatus Compiler::placeLoopAlignInstructions()
bbHavingAlign = prev;
JITDUMP("Marking " FMT_BB " before the loop with BBF_HAS_ALIGN for loop at " FMT_BB "\n", prev->bbNum,
block->bbNum);

// bbHavingAlign is in a loop, and precedes a nested loop
if (blockToLoop->GetLoop(bbHavingAlign) != nullptr)
{
// If bbHavingAlign is a removable jump, it will be removed despite it having alignment
bbHavingAlign->SetFlags(BBF_JMP_TO_NESTED_LOOP);
}
Copy link
Member

Choose a reason for hiding this comment

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

I don't think I understand what we're trying to check here, but it doesn't seem to check for any form of nestedness or relationship between the two loops.

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'm trying to check for examples like the one Kunal posted here, such that bbHavingAlign is in a loop, and precedes the start of a nested loop. I'm assuming checking if bbHavingAlign is part of a loop is sufficient?

Copy link
Member

Choose a reason for hiding this comment

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

I'm assuming checking if bbHavingAlign is part of a loop is sufficient?

Yes

Copy link
Member

Choose a reason for hiding this comment

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

Well, that case should definitely be caught but blockToLoop->GetLoop(bbHavingAlign) could potentially not be a parent loop (it might just be a block jumping backwards to a separate loop, or skipping over this loop, or exiting a previous loop into this loop). But if the worry is about affecting other loops by starting to execute alignment in them then it seems like it's orthogonal to whether or not they are nested loops. Regardless, even if we are worried about it, wouldn't the previous behavior (before BBJ_NONE removal) have been to execute their alignment?

Copy link
Member

Choose a reason for hiding this comment

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

Regardless, even if we are worried about it, wouldn't the previous behavior (before BBJ_NONE removal) have been to execute their alignment?

true, there could be occurrences where we do execute align instructions in some other loop. What we are trying to see is if we are increasing those occurrences. Once the latest diffs come out, I am happy to chat offline to discuss how to proceed with this change.

Copy link
Member Author

@amanasifkhalid amanasifkhalid Jan 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Well, that case should definitely be caught but blockToLoop->GetLoop(bbHavingAlign) could potentially not be a parent loop

I see, so we should also check that bbHavingAlign is a jump to the next block (which is the start of the loop we're aligning), right? Though that might catch the case where we exit a loop and enter another loop...

Copy link
Member

Choose a reason for hiding this comment

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

Well, that case should definitely be caught but blockToLoop->GetLoop(bbHavingAlign) could potentially not be a parent loop

I see, so we should also check that bbHavingAlign is a jump to the next block (which is the start of the loop we're aligning), right?

I think the current check is fine and we aren't really interested in nested loops vs top level loops -- just whether or not we are causing alignment to be executed in some other loop, which is what you're checking.

To answer your question regardless, you could check it using the loop tree (loop->GetParent() == blockToLoop->GetLoop(bbHavingAlign) for the immediate parent). Checking BBJ_ALWAYS would definitely make the nestedness relationship exist, but it wouldn't be a complete check (e.g. the previous block could also be a BBJ_COND/BBJ_SWITCH that skips the nested loops in some cases.

}
else
{
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/scripts/superpmi.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1971,7 +1971,7 @@ def replay_with_asm_diffs(self):

# These vars are force overridden in the SPMI runs for both the base and diff, always.
replay_vars = {
"DOTNET_JitAlignLoops": "0", # disable loop alignment to filter noise
"DOTNET_JitAlignLoops": "1", # disable loop alignment to filter noise
Copy link
Member

Choose a reason for hiding this comment

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

Don't check in this change

"DOTNET_JitEnableNoWayAssert": "1",
"DOTNET_JitNoForceFallback": "1",
}
Expand Down
Loading