JIT: Ensure the importer does not reorder GTF_ORDER_SIDEEFF with GTF_GLOB_REF - #134140
Conversation
…GLOB_REF `GTF_ORDER_SIDEEFF` should not be reordered with `GTF_GLOB_REF`. However, the importer's spill handling did not check for this case. Minimal diffs expected.
github-actions
Bot
added
the
area-CodeGen-coreclr
|
Azure Pipelines: Successfully started running 5 pipeline(s). 11 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
|
cc @dotnet/jit-contrib PTAL @EgorBo |
|
Azure Pipelines: Successfully started running 5 pipeline(s). 11 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🟡 Changes recommended
Add a focused regression test covering the ordering combination.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Updates JIT importer spill handling to preserve GTF_ORDER_SIDEEFF ordering relative to global reads.
Changes:
- Includes ordering side effects in spill analysis.
- Applies full spill checks to vectorization comparison temporaries.
File summaries
| File | Summary |
|---|---|
src/coreclr/jit/importervectorization.cpp |
Applies full spill handling to generated comparison temporaries. |
src/coreclr/jit/importer.cpp |
Preserves ordering side effects during importer spills. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
tannergooding
left a comment
tannergooding
left a comment
There was a problem hiding this comment.
LGTM. I think there's just one place we're being overly conservative, but its probably not super critical.
Given the most popular category among the 110+ bugs I filed is essentially "wrong eval order" (from AI's analysis), I suspect we'd rather be conservative 😐 |
I'm not so sure. I would guess most of the bugs are rather from historical buildup, complexity, and people working around things because the general logic is often too conservative and they can't use the existing helpers. I'd think that if we set things up to be correct and centralized, then the risk and bugs goes down substantially at the same time. --- Not saying it needs to be done here or now, just that as long as we have the main helpers being "subpar" we're going to continue having risk for future areas here. |
GTF_ORDER_SIDEEFFshould not be reordered withGTF_GLOB_REF. However, the importer's spill handling did not check for this case.Minimal diffs expected.