JIT: fix loop cloning type assumptions#128729
Conversation
Loop cloning captured array/index locals at identification time but re-read their types from lvaTable when synthesizing guard trees. If a local's tracked type no longer matches its tree-node type (e.g. TYP_REF → TYP_LONG after global morph), the generated guards have mismatched operand types and VN folds null checks incorrectly. Preserve the array ref type at identification time and use it when materializing guard comparisons. Also fix the null sentinel (always TYP_REF) to match the operand type for non-GC locals. Fixes dotnet#115663. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
This PR updates CoreCLR JIT loop cloning to stop assuming array references are always TYP_REF, by carrying forward the actual referent/local types observed during extraction and using those when synthesizing new IR for cloning conditions.
Changes:
- Track and propagate array base type (
ArrIndex::arrType) and local type (LC_Ident::lclType) through loop cloning’s symbolic condition representation. - Use the captured types when materializing new
GenTreenodes (e.g.,gtNewLclvNode) so cloned conditions remain well-typed after earlier phases rewrite local types. - Record
arrTypeduring array index extraction and inNaturalLoopIterInfo::ArrLenLimit.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/coreclr/jit/loopcloning.h | Adds type fields to loop cloning’s symbolic representations and tightens equality comparisons accordingly. |
| src/coreclr/jit/loopcloning.cpp | Uses captured types when building IR; adds logic intended to prevent mixed-type null comparisons. |
| src/coreclr/jit/flowgraph.cpp | Captures array ref type for a.Length limits in loop iteration info extraction. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The lclvars.cpp assertion requires GT_LCL_VAR node types to match lvaTable. Only IndirOfLocal needs the captured type (fixing the hardcoded TYP_REF); the null-type fixup handles the resulting mismatch in null guard comparisons. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
For jagged arrays, optExtractArrIndex is called once per dimension. The second-dimension call has a different array operand (the temp holding a[i], typed ref) not the original stack-allocated array (typed long). Setting arrType unconditionally overwrote the correct long type with ref, creating a type mismatch in the null guard. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The type compatibility checks for iter/limit vars must use lvaTable types, not tree-node types. A long local cast to int has TYP_INT in the tree but TYP_LONG in lvaTable; using the tree type bypasses the rejection and creates a TYP_LONG guard compared against a TYP_INT constant -- size mismatch assertion. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The captured tree-node type can mismatch lvaTable after object stack allocation retypes locals. Use lvaTable for both Var and IndirOfLocal; the null-type fixup in LC_Condition::ToGenTree handles the resulting TYP_LONG-vs-TYP_REF null comparison. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Enables some optimizations previously inhibited by oddly-type compares created during cloning of loops with references to stack allocated arrays. Turns out we did not need any VN fixes. @jakobbotsch PTAL |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/coreclr/jit/loopcloning.cpp:183
- LC_Ident::ToGenTree now builds
addrusing the local’s currentlvType, which can be TYP_BYREF/TYP_I_IMPL/TYP_LONG for stack-allocated objects. TheindirOffs == 0path then callsgtNewMethodTableLookup(addr)withonStack=false, butgtNewMethodTableLookupassertsonStack || object->TypeIs(TYP_REF)(compiler.hpp:1887). This will trip in DEBUG builds for non-TYP_REF locals and is likely the scenario this PR is enabling.
GenTree* addr = comp->gtNewLclvNode(lclNum, comp->lvaTable[lclNum].lvType);
if (indirOffs == 0)
{
return comp->gtNewMethodTableLookup(addr);
}
Instead of creating TYP_REF null constants and fixing them up later in LC_Condition::ToGenTree, pass the correct type to CreateNull() so the cloner creates well-typed IR from the start. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
CI test failure could be related (arm32) or maybe a different manifestation of #128595. Suspect it is unrelated. CI build failure is #128905 / #128372 / dotnet/sdk#54350 |
|
@jakobbotsch ping |
jakobbotsch
left a comment
There was a problem hiding this comment.
This looks better to me now.
|
Unblocks some loop optimizations. |
|
/ba-g unrelated failure and analyzer issue |
References to arrays may now be TYP_I_IMPL or TYP_BYREF when the referred-to arrays are / may be stack allocated. Loop cloning was always assuming them to be TYP_REF.
Update loop cloning to use the actual array referent type for the new array references it creates.
Fixes #115663.