JIT: Have physical promotion insert read backs before possible implicit control flow #86706
Conversation
Physical promotion relies on being able to read back any promoted field that is fresher in its struct local before control flows to any successor block. This was failing to take implicit control flow into account. Fix dotnet#86498
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
jakobbotsch
changed the title
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsPhysical promotion relies on being able to read back any promoted field that is fresher in its struct local before control flows to any successor block. This was failing to take implicit control flow into account. Fix #86498
|
|
cc @dotnet/jit-contrib PTAL @AndyAyersMS Small diffs. With physical promotion, without old promotion. |
| @@ -1449,11 +1583,11 @@ void ReplaceVisitor::WriteBackBefore(GenTree** use, unsigned lcl, unsigned offs, | |||
| // offs - The starting offset of the range in the struct local that needs to be read back from. | |||
| // size - The size of the range | |||
| // | |||
| void ReplaceVisitor::MarkForReadBack(unsigned lcl, unsigned offs, unsigned size) | |||
| bool ReplaceVisitor::MarkForReadBack(unsigned lcl, unsigned offs, unsigned size) | |||
There was a problem hiding this comment.
In my next change I remove this return value in favor of moving the logging into this function, so will address this with that PR.
| assert(!rep.NeedsReadBack || !rep.NeedsWriteBack); | ||
| if (rep.NeedsReadBack) | ||
| { | ||
| if (m_liveness->IsReplacementLiveOut(m_currentBlock, agg->LclNum, (unsigned)i)) |
There was a problem hiding this comment.
I suppose this is where you get into the resolution strategies... if the field is partially dead (only live in some successors) you might be tempted to defer the read back until the start of the live successors (if join free, or if all other preds of those successors will do read backs too).
We might be able to get at some of this already via tail merging.
There was a problem hiding this comment.
Yeah, I think there's a few opportunities like that around. I liked your idea of focusing on a linear trace when making these decisions.
| Compiler::fgWalkResult ReplaceVisitor::PostOrderVisit(GenTree** use, GenTree* user) | ||
| { | ||
| GenTree* tree = *use; | ||
|
|
||
| use = InsertMidTreeReadBacksIfNecessary(use); |
There was a problem hiding this comment.
Add a header comment for PostOrderVisit.
There was a problem hiding this comment.
Will add it as part of my follow-up.
…tion Handle readbacks for parameters/OSR-locals like any other readback is handled. Previously they were handled by creating the scratch BB and then inserting IR after the main replacement had already been done; now, we instead create the scratch BB eagerly and mark these as requiring a read back at the beginning of the scratch BB, and leave normal replacement logic up to handle it. The main benefit is that this unification makes it easier to ensure that future smarter handling of readbacks/writebacks (e.g. "resolution") automatically kicks in for the common case of parameters. Introduce another invariant, which is that we only ever mark a field as requiring readback if it is live. Previously we would always mark them as requiring read backs, but would then check liveness before inserting the actual IR to do the read back. But we don't always have the liveness information at the point where we insert IR for readbacks after dotnet#86706. Also expand some debug logging, and address some feedback from dotnet#86706.
…tion (#87165) Handle readbacks for parameters/OSR-locals like any other readback is handled. Previously they were handled by creating the scratch BB and then inserting IR after the main replacement had already been done; now, we instead create the scratch BB eagerly and mark these as requiring a read back at the beginning of the scratch BB, and leave normal replacement logic up to handle it. The main benefit is that this unification makes it easier to ensure that future smarter handling of readbacks/writebacks (e.g. "resolution") automatically kicks in for the common case of parameters. Introduce another invariant, which is that we only ever mark a field as requiring readback if it is live. Previously we would always mark them as requiring read backs, but would then check liveness before inserting the actual IR to do the read back. But we don't always have the liveness information at the point where we insert IR for readbacks after #86706. Also expand some debug logging, and address some feedback from #86706.
ghost
locked as resolved and limited conversation to collaborators
Physical promotion relies on being able to read back any promoted field that is fresher in its struct local before control flows to any successor block. This was failing to take implicit control flow into account.
Fix #86498