[amanasifkhalid]

Follow-up to #118463. As we walk a loop's blocks in RPO, propagate the side-effecting nature of each block to its successors, rather than flipping a single flag once side-effecting code is encountered.

[Copilot]

Pull Request Overview

This PR improves the precision of side effect propagation during loop hoisting optimization in the JIT compiler. The change replaces a simple boolean flag with a more sophisticated bit vector approach to track which blocks have encountered side effects.

• Replaces the m_beforeSideEffect boolean field with a m_sideEffectBlocks bit vector
• Tracks side effect state per block rather than globally across the entire loop
• Propagates side effect state to successor blocks when side effects are encountered

[dotnet-policy-service]

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

[amanasifkhalid]

cc @dotnet/jit-contrib, @AndyAyersMS PTAL. Diffs show PerfScore improvements outnumbering regressions in most collections, except for libraries_tests_no_tiered_compilation. Thanks!

[AndyAyersMS]

Can you call out some specific benchmarks where you expect to see perf improvements?

Can you verify any of these with local runs? Or via Egorbot?

[AndyAyersMS]

Approving but would still like to see some examples where this will resolve current perf issues.

[amanasifkhalid]

@EgorBot --arm --filter ByteMark.BenchLUDecomp

[amanasifkhalid]

@AndyAyersMS I'm not seeing as much impact in the microbenchmarks as I hoped for, mainly because this seems to impact the OSR variants of various benchmarks instead of their Tier 1 variants. Since this change looks riskier than #118463, I'm ok with holding off on this.

[AndyAyersMS]

@AndyAyersMS I'm not seeing as much impact in the microbenchmarks as I hoped for, mainly because this seems to impact the OSR variants of various benchmarks instead of their Tier 1 variants. Since this change looks riskier than #118463, I'm ok with holding off on this.

Sure, makes sense. Thanks for checking.

[JulieLeeMSFT]

@AndyAyersMS, is this ready to merge?

[AndyAyersMS]

We backed out a previous change so I'd say no. We need to look at that one and this carefully.

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "c839ac5a3539491a870f39e7dc75684a26d14a90",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "edb570cd9e75036b6c61b201a0a9a9f2301db276",
  "last_reviewed_commit": "c839ac5a3539491a870f39e7dc75684a26d14a90",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "edb570cd9e75036b6c61b201a0a9a9f2301db276",
  "last_recorded_worker_run_id": "29672720405",
  "review_attempt_commit": "",
  "review_attempt_base_ref": "",
  "review_attempt_count": 0,
  "max_review_attempts": 5,
  "review_history_format": "holistic-review-disclosure-v1",
  "review_history": [
    {
      "commit": "c839ac5a3539491a870f39e7dc75684a26d14a90",
      "review_id": 4729979993
    }
  ]
}

[github-actions]

Holistic Review

Motivation: This is a code-quality follow-up to #118463. In optHoistLoopBlocks, exception-throwing (or otherwise potentially-hoistable) expressions may only be hoisted if they execute before the first global side effect in the loop. Previously this was tracked by a single visitor-wide boolean, m_beforeSideEffect, that was flipped to false once any side effect was seen and never reset. Because loop blocks are visited in RPO, that coarse flag pessimistically treated every block visited after the first side-effecting block—including blocks on sibling control-flow paths that never actually execute after that side effect—as being "after a side effect," blocking otherwise-legal hoists.

Approach: The single m_beforeSideEffect member is replaced by a BitVec m_sideEffectBlocks (initialized empty over the existing post-order m_traits). At the start of each node's post-visit, a local beforeSideEffect is derived from whether the current block's bbPostorderNum is a member of the set. When a node establishes a side effect, the current block and each of its successors are added to m_sideEffectBlocks. Because VisitLoopBlocks iterates in reverse post order, a block's predecessors are processed before it, so a block is correctly marked "after side effect" only when it is actually reachable through a side-effecting predecessor path, rather than merely being later in RPO. Intra-block ordering is preserved: the first side-effecting node in a block adds that block to the set, so subsequent nodes in the same block recompute beforeSideEffect as false.

Summary: The transformation is correct and strictly more precise than the prior monotonic flag; it is conservative in the right direction (a block reachable from any side-effecting predecessor is marked). The bit vector is sized by the same traits used for m_defExec (full post-order count), so recording successors that lie outside the loop is harmless and in range. The change is small, well-commented, self-consistent, and confined to a single method. Diagnostics rely on JIT self-checks and SuperPMI/CI diffs rather than new tests, which is appropriate for a JIT CQ change of this nature. This already carries a JIT-maintainer approval. No correctness or performance concerns found; LGTM.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 85.9 AIC · ⌖ 14.6 AIC · ⊞ 10K