Make GT_LIST processing non-recursive to avoid StackOverflow. #7071
Conversation
|
|
||
| ftreg |= next->gtRsvdRegs; | ||
| // TODO: Do we have to compute costs differently for argument lists and |
There was a problem hiding this comment.
I don't have any real insight into this, but in my experience you would get at least some asm diffs if you had changed the cost computation.
In reply to: 77853136 [](ancestors = 77853136)
There was a problem hiding this comment.
These cost are just a best estimate of code size and execution time.
It can impact things such a places in the JIT where we would like to duplicate a piece of code.
The JIT will do so if the cost is low and omit the duplication if the cost is too high.
Other than that the costs can be off by a lot without having much impact on what code the JIT generates.
|
The mechanics look good to me, but we should take care of those TODOs before merging. |
|
The way I have it, the costs are calculated exactly the same way they were before my change. I'd like to keep this a no-diffs change. Whether we want to unify the way costs are calculated for arg lists vs. other lists is orthogonal to this change. |
|
@erozenfeld: let's file an issue to track this, then, and update the comment accordingly. |
|
It makes sense to me to file an issue on this, but it's not clear how critical an issue this would be. At this point, there aren't all that many non-call-arg uses of lists, and I suspect the costing ought to be similar, but also not sure how much it matters. In reply to: 245333065 [](ancestors = 245333065) |
|
|
We had some internal cases where crossgen failed with StackOverflow exception when compiling huge methods. In particular, the methods had GT_PHI nodes with huge number of arguments. StackOverflow was happening in multiple places. Recent LIR changes eliminated two of those places, these changes eliminate two more: gtSetEvalOrder and fgDebugCheckFlags (debug only). We already had gtSetListOrder but it was only used for call arg lists. I made gtSetListOrder non-recursive and also generalized to handle other GT_LIST nodes. With that with these changes the huge repros can now be crossgen'd. I verified no assembly diffs in SuperPMI. I'm verifying overall throughput effect.
erozenfeld
force-pushed
the
StackOverflow
branch
from
9545806
to
a67f569
Compare
|
I opened #7095 and updated the TODO comments. |
|
LGTM |
|
@dotnet-bot test OSX x64 Checked Build and Test |
|
I verified no measurable throughput impact when ngen-ing desktop assemblies. |
…ad (dotnet#7071) * Change Timer implementation on Unixes to use only one scheduling thread - Separated from dotnet/corert#7066 * Address feedback from dotnet/corert#7066 * Remove reference to s_lock * Reduce work inside lock * Move _id * Fix duplicate timers in scheduled timer list, move info to TimerQueue Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
…ad (#7071) * Change Timer implementation on Unixes to use only one scheduling thread - Separated from dotnet/corert#7066 * Address feedback from dotnet/corert#7066 * Remove reference to s_lock * Reduce work inside lock * Move _id * Fix duplicate timers in scheduled timer list, move info to TimerQueue Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
…ad (dotnet#7071) * Change Timer implementation on Unixes to use only one scheduling thread - Separated from dotnet/corert#7066 * Address feedback from dotnet/corert#7066 * Remove reference to s_lock * Reduce work inside lock * Move _id * Fix duplicate timers in scheduled timer list, move info to TimerQueue Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>
Make GT_LIST processing non-recursive to avoid StackOverflow. Commit migrated from dotnet/coreclr@871bb90
…ad (dotnet/coreclr#7071) * Change Timer implementation on Unixes to use only one scheduling thread - Separated from dotnet/corert#7066 * Address feedback from dotnet/corert#7066 * Remove reference to s_lock * Reduce work inside lock * Move _id * Fix duplicate timers in scheduled timer list, move info to TimerQueue Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com> Commit migrated from dotnet/coreclr@6e215e1
We had some internal cases where crossgen failed with StackOverflow exception
when compiling huge methods. In particular, the methods had GT_PHI nodes with
huge number of arguments.
StackOverflow was happening in multiple places. Recent LIR changes eliminated two of those places,
these changes eliminate two more: gtSetEvalOrder and fgDebugCheckFlags (debug only). We already had
gtSetListOrder but it was only used for call arg lists. I made gtSetListOrder non-recursive and also generalized
to handle other GT_LIST nodes.
With that with these changes the huge repros can now be crossgen'd.
I verified no assembly diffs in SuperPMI.
I'm verifying overall throughput effect.
@dotnet/jit-contrib PTAL