-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
JIT: improve scalability of optReachable #75990
Conversation
Use a bit vector to track the visited blocks. This scales much better than using the per-block visited flags. Fixes dotnet#44341.
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsUse a bit vector to track the visited blocks. This scales much better than using the per-block visited flags. Fixes #44341.
|
For the There are some other things we might be able to do here to speed this up even more, eg leveraging the residual bit vector state to accelerate compares starting from the same block or using the somewhat stale precomputed reachability to screen out certain cases (RBO should reduce reachability overall, never increase it). Will hold off on those for now. @jakobbotsch PTAL |
spmi diffs don't see any TP impact. This is quite surprising. I'll try a matching set of local builds and see what that says. |
I can repro the NAOT OSX x64 failure, but it is intermittent, the failure symptoms vary, and it is very likely unrelated.
@MichalStrehovsky is there a known issue here? |
We didn't look at OSX quality for 7.0 since it's unsupported in 7.0. This is probably #73299. I've not seen it in UnitTests, but who knows. We only run the tests so that we don't severely regress mac since we'll likely add it in 8.0. |
Local builds agree on minimal impact, so I guess the test case from #44341 is a real outlier. |
@@ -1619,11 +1618,13 @@ bool Compiler::optReachable(BasicBlock* const fromBlock, BasicBlock* const toBlo | |||
return true; | |||
} | |||
|
|||
if ((succ->bbFlags & BBF_VISITED) != 0) | |||
if (BitVecOps::IsMember(optReachableBitVecTraits, optReachableBitVec, succ->bbNum)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have this exact code in fgRemoveDeadBlocks()
. Can we unify them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That version uses BlockSet
, which I'm avoiding here because it is epoch sensitive.
In general, it would be nice to have the ability to represent (sparse) block sets that are independent of bbNum
and not fixed capacity so that they can persist for longer stretches or be reused as general scratch sets, but we don't have anything like that yet.
So, are you saying with this change this case still TIMEOUT? I am not surprised. It has 62348 single line |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Did you consider rewalking the graph to be able to clear just the marked subgraph?
I suppose it might not really help since pathological cases will always have to walk the majority of the flow graph, so vectorizing the clear like this might be better in practice for the outlier cases.
Another alternative could be storing an integer tag on BasicBlock
that would not need any clearing.
No, it should be ok now. I was just surprised nothing else saw any significant TP benefit from this. |
outerloop isn't clean, but only has a few failures. Let's make sure the reenabled test runs ok. |
/azp run runtime-coreclr outerloop |
Azure Pipelines successfully started running 1 pipeline(s). |
Use a bit vector to track the visited blocks. This scales much better than using the per-block visited flags.
Fixes #44341.