JIT: Fix exponential blowup of memory dependency arrays in VNForMapSelectWork #93344
Conversation
…lectWork Switch to sets when accumulating the memory dependencies found in VNForMapSelectWork. Otherwise we might duplicate each memory dependency for every path back to the store that induced it, and there can be an exponential number of those. Fix dotnet#93342
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsSwitch to sets when accumulating the memory dependencies found in Fix #93342
|
|
/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
cc @dotnet/jit-contrib PTAL @AndyAyersMS Some odd MinOpts TP diffs that I wouldn't expect, I want to check why tomorrow. |
AndyAyersMS
left a comment
There was a problem hiding this comment.
Changes LGTM, one minor comment nit.
Couldn't the MapSelectWorkCacheEntry hold a SmallValueNumSet too? Or do you think that is not worth it because that set is fixed and array rep is more space efficient?
| void ValueNumStore::MapSelectWorkCacheEntry::SetMemoryDependencies(CompAllocator alloc, | ||
| ArrayStack<ValueNum>& deps, | ||
| unsigned startIndex) | ||
| void ValueNumStore::MapSelectWorkCacheEntry::SetMemoryDependencies(Compiler* comp, SmallValueNumSet& set) |
There was a problem hiding this comment.
Update arg comment just above
Ah, yes it can, since we are creating a separate set for every recursive call now. I'll make that change and perhaps shrink the default size a bit depending on the memory overhead. |
Hmm, it has some significant ish memory overhead (1.3% more VN memory, 0.16% more memory total) since |
This reverts commit b4292d7.
Seems like MSVC stopped inlining something in Base: 447610812, Diff: 447723040, +0.0251%
?fgMorphArgs@Compiler@@AEAAPEAUGenTreeCall@@PEAU2@@Z : 67415 : +4.22% : 60.07% : +0.0151%
?AddFinalArgsAndDetermineABIInfo@CallArgs@@QEAAXPEAVCompiler@@PEAUGenTreeCall@@@Z : 43536 : +1.79% : 38.79% : +0.0097%
memcpy : 1052 : +0.25% : 0.94% : +0.0002%
memset : 225 : +0.01% : 0.20% : +0.0001%Apparently MSVC decided it is no longer profitable to inline |
|
The still running job seems to be some unrelated infra issue: Not sure why it is still showing as running. |
|
/backport to release/8.0 |
|
Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6497391189 |
Switch to sets when accumulating the memory dependencies found in
VNForMapSelectWork. Otherwise we might duplicate each memory dependencyfor every path back to the store that induced it, and there can be an
exponential number of those.
Fix #93342