Skip to content

JIT: Remove side effect detection quirk in loop hoisting #118463

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
amanasifkhalid merged 3 commits into from
Aug 12, 2025
Merged

JIT: Remove side effect detection quirk in loop hoisting #118463

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b2e6d73
Remove hoisting side effect quirk
amanasifkhalid Aug 6, 2025
217d6e7
More precise side effect propagation
amanasifkhalid Aug 6, 2025
320539e
Revert "More precise side effect propagation"
amanasifkhalid Aug 7, 2025
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
23 changes: 4 additions & 19 deletions src/coreclr/jit/optimizer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3655,21 +3655,12 @@ bool Compiler::optHoistThisLoop(FlowGraphNaturalLoop* loop, LoopHoistContext* ho
}
#endif // FEATURE_MASKED_HW_INTRINSICS

// Find the set of definitely-executed blocks.
// Find the set of definitely-executed blocks. These will be given priority for hoisting.
// Ideally, the definitely-executed blocks are the ones that post-dominate the entry block.
// Until we have post-dominators, we'll special-case for single-exit blocks.
//
// Todo: it is not clear if this is a correctness requirement or a profitability heuristic.
// It seems like the latter. Ideally there are enough safeguards to prevent hoisting exception
// or side-effect dependent things. Note that HoistVisitor uses `m_beforeSideEffect` to determine if it's
// ok to hoist a side-effect. It allows this only for the first block (the entry block), before any
// side-effect has been seen. After the first block, it assumes that there has been a side effect and
// no further side-effect can be hoisted. It is true that we don't analyze any program behavior in the
// flow graph between the entry block and the subsequent blocks, whether they be the next block dominating
// the exit block, or the pre-headers of nested loops.
//
// We really should consider hoisting from conditionally executed blocks, if they are frequently executed
// and it is safe to evaluate the tree early.
// TODO: We ought to consider hoisting more aggressively from conditionally executed blocks,
// if they are frequently executed and it is safe to evaluate the tree early.
//
assert(m_dfsTree != nullptr);
BitVecTraits traits(m_dfsTree->PostOrderTraits());
Expand Down Expand Up @@ -4292,11 +4283,6 @@ void Compiler::optHoistLoopBlocks(FlowGraphNaturalLoop* loop,

m_valueStack.Reset();
}

// Only unconditionally executed blocks in the loop are visited (see optHoistThisLoop)
// so after we're done visiting the first block we need to assume the worst, that the
// blocks that are not visited have side effects.
m_beforeSideEffect = false;
}

fgWalkResult PreOrderVisit(GenTree** use, GenTree* user)
Expand Down Expand Up @@ -4475,8 +4461,7 @@ void Compiler::optHoistLoopBlocks(FlowGraphNaturalLoop* loop,
if (!m_beforeSideEffect)
{
// For now, we give up on an expression that might raise an exception if it is after the
// first possible global side effect (and we assume we're after that if we're not in the first
// block).
// first possible global side effect.
// TODO-CQ: this is when we might do loop cloning.
//
if ((tree->gtFlags & GTF_EXCEPT) != 0)
Expand Down