JIT: Remove most loop-related hoisting quirks #95734
Conversation
We still keep the quirk that we only hoist in loops also recognized by the old loop finding. Before we can hoist in new loops we need to implement creation of preheaders, but that requires incrementally keeping the old loop table up to date, which I'd like to avoid. So my next goal is to switch everything to only have the "recognized by old loop finding" quirk so that we can remove the old loop validity requirements. A few minor diffs are expected because the new loop iteration order is not exactly the same as the old one. In particular child loop sibling links are stored in rever postorder (the old loop table has them in postorder), which means we end up hoisting them in postorder (the old one did them in reverse postorder). I initially added an `ArrayStack::Reverse` method to make it the same as previously, but the diffs were so small and just constituted some reordering, so the complication did not seem worth it.
ghost
assigned 
dotnet-issue-labeler
bot
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsWe still keep the quirk that we only hoist in loops also recognized by the old loop finding. Before we can hoist in new loops we need to implement creation of preheaders, but that requires incrementally keeping the old loop table up to date, which I'd like to avoid. So my next goal is to switch everything to only have the "recognized by old loop finding" quirk so that we can remove the old loop validity requirements. A few minor diffs are expected because the new loop iteration order is not exactly the same as the old one. In particular child loop sibling links are stored in rever postorder (the old loop table has them in postorder), which means we end up hoisting them in postorder (the old one did them in reverse postorder). I initially added an
|
|
cc @dotnet/jit-contrib PTAL @BruceForstall |
| // TODO-Quirk: Switch this to postorder over the loops, instead of this | ||
| // loop tree based thing. It is not the exact same order, but it will still | ||
| // process child loops before parent loops. |
There was a problem hiding this comment.
Ugh, forgot to remove this. Will remove it as part of a future PR.
| assert(childLoop->EntryEdges().size() == 1); | ||
| BasicBlock* childPreHead = childLoop->EntryEdge(0)->getSourceBlock(); | ||
| BasicBlock* childPreHead = childLoop->EntryEdges()[0]->getSourceBlock(); |
There was a problem hiding this comment.
...and will also fix this bad merge as part of a future PR.
We still keep the quirk that we only hoist in loops also recognized by the old loop finding. Before we can hoist in new loops we need to implement creation of preheaders, but that requires incrementally keeping the old loop table up to date, which I'd like to avoid. So my next goal is to switch everything to only have the "recognized by old loop finding" quirk so that we can remove the old loop validity requirements.
A few minor diffs are expected because the new loop iteration order is not exactly the same as the old one. In particular child loop sibling links are stored in rever postorder (the old loop table has them in postorder), which means we end up hoisting them in postorder (the old one did them in reverse postorder). I initially added an
ArrayStack::Reversemethod to make it the same as previously, but the diffs were so small and just constituted some reordering, so the complication did not seem worth it.