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: Force inline small BasicBlock methods #95484

Closed
wants to merge 2 commits into from
Closed

JIT: Force inline small BasicBlock methods #95484

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9eda59d
Force inline small BasicBlock methods
amanasifkhalid Nov 30, 2023
f934f03
Use FORCEINLINE macro
amanasifkhalid Dec 1, 2023
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
53 changes: 0 additions & 53 deletions src/coreclr/jit/block.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1630,59 +1630,6 @@ BasicBlock* BasicBlock::New(Compiler* compiler, BBjumpKinds jumpKind, unsigned j
return block;
}

//------------------------------------------------------------------------
// isBBCallAlwaysPair: Determine if this is the first block of a BBJ_CALLFINALLY/BBJ_ALWAYS pair
//
// Return Value:
// True iff "this" is the first block of a BBJ_CALLFINALLY/BBJ_ALWAYS pair
// -- a block corresponding to an exit from the try of a try/finally.
//
// Notes:
// In the flow graph, this becomes a block that calls the finally, and a second, immediately
// following empty block (in the bbNext chain) to which the finally will return, and which
// branches unconditionally to the next block to be executed outside the try/finally.
// Note that code is often generated differently than this description. For example, on ARM,
// the target of the BBJ_ALWAYS is loaded in LR (the return register), and a direct jump is
// made to the 'finally'. The effect is that the 'finally' returns directly to the target of
// the BBJ_ALWAYS. A "retless" BBJ_CALLFINALLY is one that has no corresponding BBJ_ALWAYS.
// This can happen if the finally is known to not return (e.g., it contains a 'throw'). In
// that case, the BBJ_CALLFINALLY flags has BBF_RETLESS_CALL set. Note that ARM never has
// "retless" BBJ_CALLFINALLY blocks due to a requirement to use the BBJ_ALWAYS for
// generating code.
//
bool BasicBlock::isBBCallAlwaysPair() const
Copy link
Contributor

Choose a reason for hiding this comment

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

Moving methods from a .cpp file to a header is a takeback w.r.t. maintainability (the coding convention encourages placing methods in implementation files). Does it make a good enough difference to pay for that?

Copy link
Member Author

Choose a reason for hiding this comment

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

Locally on Windows x64, I observed an additional 0.01% or 0.02% TP improvement; technically a 25-33% improvement over not force-inlining these methods, but only because the TP improvements were small on Windows x64 to begin with. As for maintainability, I agree I'd rather not move code previously in implementation files into header files, but I'm curious where we draw the line for "very small inline member function implementations" in the coding convention -- just one-line methods? Small methods with no function calls to avoid size increases from inlining?

{
if (this->KindIs(BBJ_CALLFINALLY) && !(this->bbFlags & BBF_RETLESS_CALL))
{
// Some asserts that the next block is a BBJ_ALWAYS of the proper form.
assert(!this->IsLast());
assert(this->Next()->KindIs(BBJ_ALWAYS));
assert(this->Next()->bbFlags & BBF_KEEP_BBJ_ALWAYS);
assert(this->Next()->isEmpty());

return true;
}
else
{
return false;
}
}

//------------------------------------------------------------------------
// isBBCallAlwaysPairTail: Determine if this is the last block of a BBJ_CALLFINALLY/BBJ_ALWAYS pair
//
// Return Value:
// True iff "this" is the last block of a BBJ_CALLFINALLY/BBJ_ALWAYS pair
// -- a block corresponding to an exit from the try of a try/finally.
//
// Notes:
// See notes on isBBCallAlwaysPair(), above.
//
bool BasicBlock::isBBCallAlwaysPairTail() const
{
return (bbPrev != nullptr) && bbPrev->isBBCallAlwaysPair();
}

//------------------------------------------------------------------------
// hasEHBoundaryIn: Determine if this block begins at an EH boundary.
//
Expand Down
77 changes: 61 additions & 16 deletions src/coreclr/jit/block.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -538,7 +538,7 @@ struct BasicBlock : private LIR::Range
return bbJumpKind;
}

void SetJumpKind(BBjumpKinds jumpKind)
__forceinline void SetJumpKind(BBjumpKinds jumpKind)
Copy link
Contributor

Choose a reason for hiding this comment

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

__forceinline is an MSVC-original construct. It works, but it would be more idiomatic to use the FORCEINLINE macro.

{
// If this block's jump kind requires a target, ensure it is already set
assert(!HasJumpDest() || HasInitializedJumpDest());
Expand All @@ -561,7 +561,7 @@ struct BasicBlock : private LIR::Range
}
}

BasicBlock* Next() const
__forceinline BasicBlock* Next() const
{
return bbNext;
}
Expand Down Expand Up @@ -619,7 +619,7 @@ struct BasicBlock : private LIR::Range
return KindIs(BBJ_ALWAYS, BBJ_CALLFINALLY, BBJ_COND, BBJ_EHCATCHRET, BBJ_EHFILTERRET, BBJ_LEAVE);
}

BasicBlock* GetJumpDest() const
__forceinline BasicBlock* GetJumpDest() const
{
// If bbJumpKind indicates this block has a jump, bbJumpDest cannot be null
assert(!HasJumpDest() || HasInitializedJumpDest());
Expand All @@ -634,7 +634,7 @@ struct BasicBlock : private LIR::Range
assert(!HasJumpDest() || HasInitializedJumpDest());
}

void SetJumpKindAndTarget(BBjumpKinds jumpKind, BasicBlock* jumpDest = nullptr)
__forceinline void SetJumpKindAndTarget(BBjumpKinds jumpKind, BasicBlock* jumpDest = nullptr)
{
bbJumpKind = jumpKind;
bbJumpDest = jumpDest;
Expand All @@ -655,7 +655,7 @@ struct BasicBlock : private LIR::Range
return (bbJumpDest == jumpDest);
}

bool JumpsToNext() const
__forceinline bool JumpsToNext() const
{
assert(HasInitializedJumpDest());
return (bbJumpDest == bbNext);
Expand Down Expand Up @@ -705,7 +705,7 @@ struct BasicBlock : private LIR::Range
unsigned bbRefs; // number of blocks that can reach here, either by fall-through or a branch. If this falls to zero,
// the block is unreachable.

bool isRunRarely() const
__forceinline bool isRunRarely() const
{
return ((bbFlags & BBF_RUN_RARELY) != 0);
}
Expand Down Expand Up @@ -831,7 +831,7 @@ struct BasicBlock : private LIR::Range

// Set block weight to zero, and set run rarely flag.
//
void bbSetRunRarely()
__forceinline void bbSetRunRarely()
{
this->scaleBBWeight(BB_ZERO_WEIGHT);
}
Expand Down Expand Up @@ -869,15 +869,60 @@ struct BasicBlock : private LIR::Range

bool isValid() const;

// Returns "true" iff "this" is the first block of a BBJ_CALLFINALLY/BBJ_ALWAYS pair --
// a block corresponding to an exit from the try of a try/finally.
bool isBBCallAlwaysPair() const;
//------------------------------------------------------------------------
// isBBCallAlwaysPair: Determine if this is the first block of a BBJ_CALLFINALLY/BBJ_ALWAYS pair
//
// Return Value:
// True iff "this" is the first block of a BBJ_CALLFINALLY/BBJ_ALWAYS pair
// -- a block corresponding to an exit from the try of a try/finally.
//
// Notes:
// In the flow graph, this becomes a block that calls the finally, and a second, immediately
// following empty block (in the bbNext chain) to which the finally will return, and which
// branches unconditionally to the next block to be executed outside the try/finally.
// Note that code is often generated differently than this description. For example, on ARM,
// the target of the BBJ_ALWAYS is loaded in LR (the return register), and a direct jump is
// made to the 'finally'. The effect is that the 'finally' returns directly to the target of
// the BBJ_ALWAYS. A "retless" BBJ_CALLFINALLY is one that has no corresponding BBJ_ALWAYS.
// This can happen if the finally is known to not return (e.g., it contains a 'throw'). In
// that case, the BBJ_CALLFINALLY flags has BBF_RETLESS_CALL set. Note that ARM never has
// "retless" BBJ_CALLFINALLY blocks due to a requirement to use the BBJ_ALWAYS for
// generating code.
//
__forceinline bool isBBCallAlwaysPair() const
{
if (this->KindIs(BBJ_CALLFINALLY) && !(this->bbFlags & BBF_RETLESS_CALL))
{
// Some asserts that the next block is a BBJ_ALWAYS of the proper form.
assert(!this->IsLast());
assert(this->Next()->KindIs(BBJ_ALWAYS));
assert(this->Next()->bbFlags & BBF_KEEP_BBJ_ALWAYS);
assert(this->Next()->isEmpty());

return true;
}
else
{
return false;
}
}

// Returns "true" iff "this" is the last block of a BBJ_CALLFINALLY/BBJ_ALWAYS pair --
// a block corresponding to an exit from the try of a try/finally.
bool isBBCallAlwaysPairTail() const;
//------------------------------------------------------------------------
// isBBCallAlwaysPairTail: Determine if this is the last block of a BBJ_CALLFINALLY/BBJ_ALWAYS pair
//
// Return Value:
// True iff "this" is the last block of a BBJ_CALLFINALLY/BBJ_ALWAYS pair
// -- a block corresponding to an exit from the try of a try/finally.
//
// Notes:
// See notes on isBBCallAlwaysPair(), above.
//
__forceinline bool isBBCallAlwaysPairTail() const
{
return (bbPrev != nullptr) && bbPrev->isBBCallAlwaysPair();
}

bool KindIs(BBjumpKinds kind) const
__forceinline bool KindIs(BBjumpKinds kind) const
{
return bbJumpKind == kind;
}
Expand Down Expand Up @@ -1016,11 +1061,11 @@ struct BasicBlock : private LIR::Range
// catch type: class token of handler, or one of BBCT_*. Only set on first block of catch handler.
unsigned bbCatchTyp;

bool hasTryIndex() const
__forceinline bool hasTryIndex() const
{
return bbTryIndex != 0;
}
bool hasHndIndex() const
__forceinline bool hasHndIndex() const
{
return bbHndIndex != 0;
}
Expand Down
Loading