JIT: Preference SDSU intervals away from killed registers#125219
JIT: Preference SDSU intervals away from killed registers#125219jakobbotsch wants to merge 10 commits intodotnet:mainfrom
Conversation
A def-use conflict happens for SDSU intervals when the def and use share no common register. For these cases we run through a set of potential ways to resolve the conflict that make use of the contract between LSRA and codegen. For example, when the definition requires a single fixed register, it is still valid for LSRA to assign a different register. This means that codegen can use the original fixed register, but that it must insert a copy into the register assigned by LSRA. In the worst case we cannot use any of the approaches to solve the conflict and then we will fall back to spilling/copying as necessary. It appears that several of the cases were overly conservative. Particularly case 1 and 2, as described by the function header, were never reachable; we assign `defRegConflict` and `useRegConflict` based on whether there is a conflict in register constraints, but that is always true when we get into this function. It appears the original intention of these variables were whether there is a conflict with a _different_ use of the registers we could otherwise have resolved the original conflict with. This PR restores the meaning to that.
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
There was a problem hiding this comment.
Pull request overview
Revives prior LSRA heuristics so SDSU (single-def/single-use) temp intervals are preferentially allocated away from registers killed by the consuming node, addressing regressions observed after updated def/use conflict handling.
Changes:
- Reworks
resolveConflictingDefAndUseconflict tracking so cases like #5 become reachable again and adjusts fixed-reg handling in case #3. - Refactors kill-driven preference updates into
updateIntervalPreferencesForKilland applies it to both live locals and in-flight (defList) non-local intervals. - Adds the new helper declaration to
lsra.h.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/coreclr/jit/lsrabuild.cpp |
Updates SDSU def/use conflict resolution logic; refactors and extends kill-based register preference updates; adds helper implementation. |
src/coreclr/jit/lsra.h |
Declares the new updateIntervalPreferencesForKill helper on LinearScan. |
Comments suppressed due to low confidence (1)
src/coreclr/jit/lsrabuild.cpp:347
- In case #4 we update
useRefPosition->registerAssignmentbut keepuseRefPosition->isFixedRegReftrue.RegisterSelection::select()asserts thatisFixedRegRefimplies a single-bitregisterAssignment, and keeping the fixed flag also means we’re effectively changing the fixed register requirement todefRegAssignment. With the new meaning ofdefRegConflict, this branch can now execute whendefRegAssignmentis multi-reg (or just different from the original fixed reg), which can violate the fixed-reg invariant and/or cause incorrect codegen assumptions. Consider either requiringdefRefPosition->isFixedRegRef/isSingleRegister(defRegAssignment)for case #4, or clearinguseRefPosition->isFixedRegRef(similar to case #3/#5) when you change its candidates away from the fixed register.
if ((useReg != REG_NA) && !defRegConflict && canChangeUseAssignment)
{
// This is case #4.
INDEBUG(dumpLsraAllocationEvent(LSRA_EVENT_DEFUSE_CASE4, interval));
useRefPosition->registerAssignment = defRegAssignment;
return;
| // Handled via liveness above | ||
| continue; | ||
| } | ||
|
|
There was a problem hiding this comment.
The new defList iteration applies updateIntervalPreferencesForKill to all non-local intervals, including float/double temps. That bypasses the existing heuristic in this method that intentionally avoids setting callee-save preferences for floating-point (see the comment just above and the fpCalleeSaveCandidateVars filtering for locals). As written, float/double LIR edges can now get preferCalleeSave set and have their preferences restricted away from call-killed regs, which may increase use of FP callee-saves and grow prologs/epilogs.
Consider applying the same filtering here as for locals (e.g., skip when the interval type is float/double unless you have a separate candidate heuristic for non-locals), or adjust updateIntervalPreferencesForKill to not set preferCalleeSave / not update prefs for float/double intervals when handling call kills.
| // Do not update callee-save preferences for floating-point intervals. | |
| // This mirrors the heuristic used for locals above, which avoids setting | |
| // preferCalleeSave for most float/double values to prevent overuse of | |
| // FP callee-save registers and larger prologs/epilogs. | |
| if (varTypeIsFloating(interval->registerType)) | |
| { | |
| continue; | |
| } |
There was a problem hiding this comment.
The downside is that we end up saving/restoring a callee saved float reg in prolog/epilog, which costs more than only spilling it in one place. The upside is that in many cases, the prolog/epilogs are cheaper than the spill that would otherwise happen inside the function.
Given how ad-hoc the existing heuristic seems I think I will leave this, if we see benchmark regressions at least we'll have some cases to look at for designing a heuristic.
This revives #81242 on top of #125214. I noticed some regressions with #125214 because we were missing this logic (specifically case 5 when resolving DU conflicts). Also, this may have some TP implications that I want to look at separately from the other change.
Example: