-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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: Update type when return temp is freshly created #111948
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
Benchmark result:
|
Co-authored-by: Jakob Botsch Nielsen <[email protected]>
Just noting that this PR is handling the case where the type is obtained via inlining; the actual deabstraction when the type is known was done as part of #108913. This unlocks that optimization for more cases. |
The test seems only fail on Linux-musl-arm, and I'm not able to reproduce it in my local environment (x64). From the test log I'm not sure whether it's introduced by this PR because I don't get how could it be related, or it's an existing bug that gets exposed by this PR. |
After updating to the latest branch, the failed tests passed this time. |
Also we should run jitstress and maybe some pgo stress on this, so it gets exposed to different inlining patterns. |
CI is passing. (The remaining osx-arm64 two are likely to timeout like all other recent PRs). Not too much interesting results from Diffs due to a lot of missing contexts. TP impact is not observable. Diffs from MihuBot: https://gist.github.com/MihuBot/1674fcd3253ccef9fa1b24e541a0ab1d |
Seems that the issue only happening on dynamic for COM objects. Deployed a workaround for this. (Or maybe we can ignore this one because it's too niche?) |
This reverts commit e94839b.
90c769a
to
7844b8f
Compare
Okay. After a few hours of investigation, I scoped the issue to
So here cc: @jakobbotsch |
So I think the issue is in the importer: runtime/src/coreclr/jit/importer.cpp Lines 11066 to 11087 in 7afd807
We didn't check the difference of the type exactness here. If we see multiple sites have different exactness, we should reset it. |
Yes, that looks like the actual problem to me as well. |
Tests should pass this time. PTAL. |
/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress, runtime-coreclr pgostress |
Azure Pipelines successfully started running 3 pipeline(s). |
Test failure seems unrelated. |
Failures due to #112021. |
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.
Thanks, this looks good.
Not clear why build analysis is red here. I think this change is good. but let's wait for the current CI problem to get resolved and merge up and run the tests once more. |
It seems that the infra issue is causing every PR to fail the CI. Looking at the logs it seems that the tests are passing, but the test environment is lacking some python dependencies so that the result was failed to upload correctly. |
Yep. We are expecting the CI issues to be fixed fairly soon, and I will trigger new tests when that happens. |
Should be fixed... |
/azp run runtime-coreclr pgostress |
Azure Pipelines successfully started running 1 pipeline(s). |
Test failures are unrelated. |
Thanks again @hez2010 ! |
Together with #110827 and things we have done for #108913 so far, this unblocks us to fully de-abstract array enumerators even without PGO.
Contributes to #108913.
Before:
After: