Skip to content

JIT: Allow forward sub to reorder trees throwing the same single exception #85647

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

Merged
merged 3 commits into from
May 4, 2023
Merged

JIT: Allow forward sub to reorder trees throwing the same single exception #85647

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
019d8dc
JIT: Allow forward sub to reorder trees throwing same exceptions
jakobbotsch May 2, 2023
6f63746
Revert unrelated change
jakobbotsch May 2, 2023
7ec53b1
Address feedback
jakobbotsch May 3, 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
106 changes: 76 additions & 30 deletions src/coreclr/jit/forwardsub.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,16 +190,7 @@ class ForwardSubVisitor final : public GenTreeVisitor<ForwardSubVisitor>
};

ForwardSubVisitor(Compiler* compiler, unsigned lclNum, bool livenessBased)
: GenTreeVisitor(compiler)
, m_use(nullptr)
, m_node(nullptr)
, m_parentNode(nullptr)
, m_lclNum(lclNum)
, m_parentLclNum(BAD_VAR_NUM)
, m_useFlags(GTF_EMPTY)
, m_accumulatedFlags(GTF_EMPTY)
, m_treeSize(0)
, m_livenessBased(livenessBased)
: GenTreeVisitor(compiler), m_lclNum(lclNum), m_livenessBased(livenessBased)
{
LclVarDsc* dsc = compiler->lvaGetDesc(m_lclNum);
if (dsc->lvIsStructField)
Expand Down Expand Up @@ -238,10 +229,11 @@ class ForwardSubVisitor final : public GenTreeVisitor<ForwardSubVisitor>

if (!isDef && !isCallTarget && IsLastUse(node->AsLclVar()))
{
m_node = node;
m_use = use;
m_useFlags = m_accumulatedFlags;
m_parentNode = parent;
m_node = node;
m_use = use;
m_useFlags = m_accumulatedFlags;
m_useExceptions = m_accumulatedExceptions;
m_parentNode = parent;
}
}
}
Expand Down Expand Up @@ -274,6 +266,20 @@ class ForwardSubVisitor final : public GenTreeVisitor<ForwardSubVisitor>
}

m_accumulatedFlags |= (node->gtFlags & GTF_GLOB_EFFECT);
if ((node->gtFlags & GTF_CALL) != 0)
Copy link
Member

@AndyAyersMS AndyAyersMS May 3, 2023

Choose a reason for hiding this comment

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

Seems odd that OperExceptions doesn't really handle GT_CALL. I wonder if there's any gain to be had by allowing some helper calls to forward sub... probably not much.

Copy link
Member Author

@jakobbotsch jakobbotsch May 3, 2023

Choose a reason for hiding this comment

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

This was intentional when I first added this API. It's really because of what @markples mentions below: OperExceptions does not make sense for user calls. The return value of ExceptionSetFlags::All indicates that something can throw all the built-in exceptions, but user calls can throw many more exceptions types. So to model that we would probably add some ExceptionSetFlags::UnknownException, but then the callers would need to either handle it specially or assert that they didn't see it.
The handling here by setting All is really just relying on forward-sub specific knowledge that the caller is going to handle that in the correct way, it's not actually correct to say that GTF_CALL nodes can throw those exceptions only.

I wonder if there's any gain to be had by allowing some helper calls to forward sub... probably not much.

Definitely something we could try. We would need to generalize things a bit here to track more precise effects, since at that point we are throwing away the "GTF_CALL means user call" assumption that the current interference checks in the caller are relying on. (But that might also make sense if we are going to end up with a GT_BOX that has GTF_CALL set on it in the future.)

{
m_accumulatedExceptions = ExceptionSetFlags::All;
}
else if ((node->gtFlags & GTF_EXCEPT) != 0)
{
// We can never reorder in the face of different exception types,
// so stop calling 'OperExceptions' once we've seen more than one
// different exception type.
if (genCountBits(static_cast<uint32_t>(m_accumulatedExceptions)) <= 1)
{
m_accumulatedExceptions |= node->OperExceptions(m_compiler);
}
}

return fgWalkResult::WALK_CONTINUE;
}
Expand Down Expand Up @@ -305,6 +311,11 @@ class ForwardSubVisitor final : public GenTreeVisitor<ForwardSubVisitor>
return m_useFlags;
}

ExceptionSetFlags GetExceptions() const
Copy link
Contributor

@markples markples May 3, 2023

Choose a reason for hiding this comment

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

It's probably worth additional comments here and/or on m_useExceptions/m_accumulatedExceptions that a value with >1 exceptions isn't trustworthy to avoid some accidental misuse.

Copy link
Member Author

@jakobbotsch jakobbotsch May 3, 2023

Choose a reason for hiding this comment

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

Will add that.

{
return m_useExceptions;
}

bool IsCallArg() const
{
return m_parentNode->IsCall();
Expand Down Expand Up @@ -366,18 +377,20 @@ class ForwardSubVisitor final : public GenTreeVisitor<ForwardSubVisitor>
}

private:
GenTree** m_use;
GenTree* m_node;
GenTree* m_parentNode;
GenTree** m_use = nullptr;
GenTree* m_node = nullptr;
GenTree* m_parentNode = nullptr;
unsigned m_lclNum;
unsigned m_parentLclNum;
unsigned m_parentLclNum = BAD_VAR_NUM;
#ifdef DEBUG
unsigned m_useCount = 0;
#endif
GenTreeFlags m_useFlags;
GenTreeFlags m_accumulatedFlags;
unsigned m_treeSize;
bool m_livenessBased;
GenTreeFlags m_useFlags = GTF_EMPTY;
GenTreeFlags m_accumulatedFlags = GTF_EMPTY;
ExceptionSetFlags m_useExceptions = ExceptionSetFlags::None;
ExceptionSetFlags m_accumulatedExceptions = ExceptionSetFlags::None;
unsigned m_treeSize = 0;
bool m_livenessBased;
};

//------------------------------------------------------------------------
Expand Down Expand Up @@ -689,23 +702,56 @@ bool Compiler::fgForwardSubStatement(Statement* stmt)
//
// if the next tree can't change the value of fwdSubNode or be impacted by fwdSubNode effects
//
const bool fwdSubNodeInvariant = ((fwdSubNode->gtFlags & GTF_ALL_EFFECT) == 0);
const bool nextTreeIsPureUpToUse = ((fsv.GetFlags() & (GTF_EXCEPT | GTF_GLOB_REF | GTF_CALL)) == 0);
if (!fwdSubNodeInvariant && !nextTreeIsPureUpToUse)
if (((fwdSubNode->gtFlags & GTF_CALL) != 0) && ((fsv.GetFlags() & GTF_ALL_EFFECT) != 0))
{
// Fwd sub may impact global values and or reorder exceptions...
//
JITDUMP(" potentially interacting effects\n");
JITDUMP(" cannot reorder call with any side effect\n");
return false;
}
if (((fwdSubNode->gtFlags & GTF_GLOB_REF) != 0) && ((fsv.GetFlags() & GTF_PERSISTENT_SIDE_EFFECTS) != 0))
{
JITDUMP(" cannot reorder global reference with persistent side effects\n");
return false;
}
if (((fwdSubNode->gtFlags & GTF_ORDER_SIDEEFF) != 0) &&
((fsv.GetFlags() & (GTF_GLOB_REF | GTF_ORDER_SIDEEFF)) != 0))
{
JITDUMP(" cannot reorder ordering side effect with global reference/ordering side effect\n");
return false;
}
if ((fwdSubNode->gtFlags & GTF_EXCEPT) != 0)
{
if ((fsv.GetFlags() & GTF_PERSISTENT_SIDE_EFFECTS) != 0)
{
JITDUMP(" cannot reorder exception with persistent side effect\n");
return false;
}

if ((fsv.GetFlags() & GTF_EXCEPT) != 0)
{
assert(fsv.GetExceptions() != ExceptionSetFlags::None);
if (genCountBits(static_cast<uint32_t>(fsv.GetExceptions())) > 1)
{
JITDUMP(" cannot reorder different thrown exceptions\n");
return false;
}

ExceptionSetFlags fwdSubNodeExceptions = gtCollectExceptions(fwdSubNode);
assert(fwdSubNodeExceptions != ExceptionSetFlags::None);
if (fwdSubNodeExceptions != fsv.GetExceptions())
{
JITDUMP(" cannot reorder different thrown exceptions\n");
return false;
}
}
}

// If we're relying on purity of fwdSubNode for legality of forward sub,
// do some extra checks for global uses that might not be reflected in the flags.
//
// TODO: remove this once we can trust upstream phases and/or gtUpdateStmtSideEffects
// to set GTF_GLOB_REF properly.
//
if (fwdSubNodeInvariant && ((fsv.GetFlags() & (GTF_CALL | GTF_ASG)) != 0))
if ((fsv.GetFlags() & GTF_PERSISTENT_SIDE_EFFECTS) != 0)
{
EffectsVisitor ev(this);
ev.WalkTree(&fwdSubNode, nullptr);
Expand Down Expand Up @@ -874,7 +920,7 @@ bool Compiler::fgForwardSubStatement(Statement* stmt)
fgForwardSubUpdateLiveness(firstLcl, lhsNode->gtPrev);
}

if (!fwdSubNodeInvariant)
if ((fwdSubNode->gtFlags & GTF_ALL_EFFECT) != 0)
{
gtUpdateStmtSideEffects(nextStmt);
}
Expand Down