-
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
Cache allregs to avoid checking the type repeatedly #76850
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue Detailsnull
|
@dotnet/jit-contrib |
What's up with the regressions on x86? |
Note it is present in Windows x86/MinOpts benchmarks / CG collections as well. |
I wonder if it's because it makes |
Ok, yeah I am still learning that I have to expand those details :) Tried inlining the |
Still need to do the TP regression analysis for MinOpts. |
Ping to myself. |
Hoping to review the TP analysis soon. |
The biggest diffs in MinOpts for benchmarks on windows/x64 are these. I will investigate more. -22555 : ??$varTypeIsSIMD@W4var_types@@@@YA_NW4var_types@@@Z
+193049 : ??$varTypeIsSIMD@W4var_types@@@@YA_NW4var_types@@@Z
-460532 : ??0LinearScan@@QEAA@PEAVCompiler@@@Z
+942354 : ??0LinearScan@@QEAA@PEAVCompiler@@@Z
|
/azp run runtime-coreclr superpmi-diffs |
Azure Pipelines successfully started running 1 pipeline(s). |
The latest run doesn't have any benchmarks regression. It just has TP regression on crossgen2 and I tried to eliminate some of that by eliminating the call to Here is the latest and I don't think there is anything much that can be done:
|
@kunalspathak What if you change the initialization of the table to: for (unsigned int i = 0; i < TYP_COUNT; i++)
{
availableRegs[i] = &availableIntRegs;
}
availableRegs[TYP_FLOAT] = &availableFloatRegs;
availableRegs[TYP_DOUBLE] = &availableDoubleRegs;
#ifdef FEATURE_SIMD
availableRegs[TYP_SIMD8] = &availableDoubleRegs;
availableRegs[TYP_SIMD12] = &availableDoubleRegs;
availableRegs[TYP_SIMD16] = &availableDoubleRegs;
availableRegs[TYP_SIMD32] = &availableDoubleRegs;
#endif In any case it still looks minor to me. |
That's a reasonable approach, but as I mentioned earlier, this is This was a good exercise for me to learn how to analyze TP regression locally. |
Cache the
availableRegs
in a table that we can look-up and return instead of comparing the type every single time inallRegs()
method. Gives a nice TP: