-
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
Move arithmetic DivideByZero and Overflow exceptions to JIT on 32-bit platforms #110226
Comments
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
@EgorBo Any chance you can take a look at this? This is contributing to our larger runtime simplification and unifications efforts. |
Can we just introduce managed helpers? (with qcalls into native helpers for actual division, or maybe even fully managed). |
@EgorBo I was wondering if we can handle it in JIT itself, like we are checking the conditions here: |
@jkotas @am11 it looks like we need to emit quite a bit of control-flow to re-create what these conditions do https://github.com/am11/runtime/blob/0e61d9d6ca5c312443edf175e7d7b6be72077c00/src/libraries/System.Private.CoreLib/src/System/Math.DivModInt.cs#L55-L78 (4 different helpers + extra helpers on arm32) and there is little we can re-use from existing arm64 impl. After discussing this with @jakobbotsch, we wonder if we can:
also, I've kicked off a benchmark on 32bit windows to see performance impact of am11 impl: #109087 (comment) |
Do you mean to build a general-purpose feature to inline JIT helpers that happen to be written in C#?
It would require depending on undocumented C runtime details that's problematic. I do not think we need it to avoid regressions. We are paying for the FCall wrapper today. We do not need to be doing extra work to improve x86.
It shows 1.7x regression. It is more than the earlier measurements that showed about 30% regression. |
It looks like the codegen for the C# version is not perfect (and e.g. Is32bitSigned is not inlined). Not sure we want to invest into peepholes for 32bit, though.
Yep, I think we wanted that elsewhere too Overall, I don't have a strong opinion on which path to choose, but looks like all of them involve quite a bit of work:
Probably, the 1st one indeed is the simplest (if we also avoid smart optimizations with "is it actually 32bit value") |
Div/Mod helpers in jithelpers.cpp are only called on 32-bit platforms. RV64 and LA64 seem to have a little bit different morphing: runtime/src/coreclr/jit/morph.cpp Line 8582 in c6bebf0
|
Looks like you settled on inlining the helpers as IL. Sounds good to me! |
To be fair, that is unrelated effort 🙂 just that we wanted it for several things in the past. In this case, all 3 options have pros/cons. Helpers will inline control-flow for all DIV operations early and we won't be able to hoist it/etc. |
I do not see it as a problem for the DIV/MOD helpers on 32-bit platforms. These operations have side-effects (throw exceptions). I would expect that chances of them being hoisted in real-world code are fairly low. It may not be desirable for other types of helpers. |
@EgorBo, I think this is the simplest option. JIT is already injecting DivByZero and Overflow exceptions in some cases ( |
From @jkotas #109087 (comment)
On 32 bit platforms such as x86 and arm32, JIT uses software fallback
CORINFO_HELP_{,U,L,UL}{DIV,MOD}
to handleDivideByZero
andOverflow
exceptions in div/mod arithmetic ops viaFCThrow
in jithelpers.cpp.FCThrow
usesHELPER_METHOD_FRAME
which we are trying to remove from runtime (#95695).JIT can handle inserting the software fallback using the existing
CORINFO_HELP_OVERFLOW
andCORINFO_HELP_THROWDIVZERO
helpers on 32 bit platforms. This will remove 8 (out of 9) remainingFCThrow
calls from jithelpers.cpp.The text was updated successfully, but these errors were encountered: