Alternate approach for HELPER_METHOD_FRAME removal for arithmetic divide (take2) #114352
Conversation
…metic div…" (dotnet#114308) This reverts commit 367e0a8.
|
@am11 this should fix the build issues. |
There was a problem hiding this comment.
Copilot reviewed 18 out of 22 changed files in this pull request and generated 2 comments.
Files not reviewed (4)
- src/coreclr/classlibnative/float/CMakeLists.txt: Language not supported
- src/coreclr/nativeaot/Runtime/i386/AsmMacros.inc: Language not supported
- src/coreclr/nativeaot/System.Private.CoreLib/src/System.Private.CoreLib.csproj: Language not supported
- src/coreclr/vm/i386/asmhelpers.asm: Language not supported
|
|
||
| #include <optsmallperfcritical.h> | ||
|
|
||
| FCIMPL2(int32_t, DivModInt::DivInt32, int32_t dividend, int32_t divisor) |
There was a problem hiding this comment.
Division functions rely solely on ASSERT for a zero divisor check, which may be compiled out in release builds. Consider adding an explicit runtime check to ensure safe behavior in production.
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
| @@ -66,7 +66,6 @@ public static void GetEntryPoint(TypeSystemContext context, ReadyToRunHelper id, | |||
| { | |||
| TargetArchitecture.ARM64 => "RhpAssignRefArm64", | |||
| TargetArchitecture.LoongArch64 => "RhpAssignRefLoongArch64", | |||
| TargetArchitecture.RiscV64 => "RhpAssignRefRiscV64", | |||
There was a problem hiding this comment.
why on earth did the revert command do that
There was a problem hiding this comment.
It was in the first PR as well. I think it was copied from old branch when we didn't had riscv64 handling. :D
There was a problem hiding this comment.
Ah, most likely. Now I'm just moderately cranky, not infinitely cranky.
src/coreclr/jit/valuenum.cpp
Outdated
| case CORINFO_HELP_LMOD: | ||
| case CORINFO_HELP_MOD: |
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
src/libraries/System.Private.CoreLib/src/System/Math.DivModInt.cs
Outdated
Show resolved
Hide resolved
| @@ -156,6 +156,7 @@ RhpTrapThreads equ _RhpTrapThreads | |||
| RhpStressGc equ @RhpStressGc@0 | |||
| RhpGcPoll2 equ @RhpGcPoll2@4 | |||
| RhHandleGet equ @RhHandleGet@4 | |||
| DivInt64Internal equ @DivInt64Internal_FCall@16 | |||
There was a problem hiding this comment.
Where is this used? I am not able to see it by just looking at the code.
There was a problem hiding this comment.
It's some sort of voodoo magic linker goop which makes the alternate name stuff that @filipnavara added about a month ago work. The change a few lines below which references the DivInt64Internal function is what uses this particular
line. The comments above that are ... the description I was working off of which really seems like a manifestation of some sort of linker bug, but it DID fix the build reliably on my machine.
There was a problem hiding this comment.
...voodoo magic linker goop which makes the alternate name stuff that @filipnavara added about a month ago work.
More like a year ago. 😅 The alternate name linker directives only work if anything references any symbol in the file with them. Otherwise the linker discards the whole object file before looking at the linker directives embedded inside.
Co-authored-by: Jan Kotas <[email protected]>
|
/azp run runtime-nativeaot-outerloop |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| @@ -86,11 +86,6 @@ | |||
| // COMPlusThrow(execpt); | |||
| // HELPER_METHOD_FRAME_END() | |||
|
|
|||
| // It is more efficient (in space) to use convenience macro FCTHROW that does | |||
There was a problem hiding this comment.
Nit: The whole paragraph can be deleted - it is unfinished sentence now. (It does not matter a whole lot - I expect the whole file is going to be deleted soon.)
|
/ba-g The only failure is a known test issue which the known issues logic seems to be having difficulty with. |
This is a combination of @am11's work in PR #109087 and some work to just rewrite the Windows X86 helpers in assembly.
For non 64-bit platforms, remove the helpers
For Unix platforms, rely on an implementation which uses an FCall to native code to invoke the various operations.
For Windows X86 on CoreCLR, rewrite the helpers in assembly with a tail-call approach for throwing.
Also it was noted that the existing helpers do a fair bit of unnecessary stack manipulation, and the manual rewrite avoids all of that. Maybe this will actually be faster. (Turns out it is about the same on performance. The real win would come from re-ordering the argument order, but that's a much more invasive change in the JIT, which I judge to be too risky)
Revert #114308 and fix the X86 NativeAOT build