[LoongArch64] Enable Runtime Async. (issue#124935)#125114
[LoongArch64] Enable Runtime Async. (issue#124935)#125114LuckyXu-HF wants to merge 4 commits intodotnet:mainfrom
Conversation
* Enable Runtime Async of LoongArch64 dotnet#124935 * Fix up hijacking on loongarch64 (preserve async continuation register). * Revert `lvaAsyncExecutionContextVar, lvaAsyncSynchronizationContextVar` related offset according to LA64's frame layout.
| if ((lvaAsyncExecutionContextVar != BAD_VAR_NUM) && !opts.IsOSR()) | ||
| { | ||
| int offset = lvaTable[lvaAsyncExecutionContextVar].GetStackOffset() + delta; | ||
| lvaTable[lvaAsyncExecutionContextVar].SetStackOffset(offset); | ||
| delta += lvaLclStackHomeSize(lvaAsyncExecutionContextVar); | ||
| } | ||
|
|
||
| if ((lvaAsyncSynchronizationContextVar != BAD_VAR_NUM) && !opts.IsOSR()) | ||
| { | ||
| int offset = lvaTable[lvaAsyncSynchronizationContextVar].GetStackOffset() + delta; | ||
| lvaTable[lvaAsyncSynchronizationContextVar].SetStackOffset(offset); | ||
| delta += lvaLclStackHomeSize(lvaAsyncSynchronizationContextVar); | ||
| } | ||
|
|
There was a problem hiding this comment.
For EnC to work it is necessary that this is allocated in the top of the stack frame and included as part of the EnC frame header (the code you deleted under the m_compiler->opts.compDbgEnC in the PR). This is similar to how lvaMonAcquired is handled.
I am ok with it, but just know that EnC will not work correctly without this.
There was a problem hiding this comment.
Thank you very much for point this out!
I have some questions about this, could you please give me some suggestions?
-
Is the EnC request to enble
FEATURE_REMAP_FUNCTION? If so I think LA64 will not hit the EnC scenes temporarily. (the code under them_compiler->opts.compDbgEnCinCodeGen::genCreateAndStoreGCInfolooks improper for LA64 now. I will delete and add an assert at it.) -
Are there any testing related to EnC? We can add EnC support for LA64 if necessary later. ( I used to run a hot reload demo at .NET6 and it's ok on LA64 at that time: https://github.com/jsuarezruiz/AvaloniaSkiaSharpFiddle ).
For EnC to work it is necessary that this is allocated in the top of the stack frame and included as part of the EnC frame header (the code you deleted under the m_compiler->opts.compDbgEnC in the PR). This is similar to how lvaMonAcquired is handled.
Thank you! If I understand correctly lvaAsyncExecutionContextVar and lvaAsyncExecutionContextVar are temp locals, on loongarch64 (and maybe on risc-v, as similar frame layout) if place this after lvaMonAcquired, the temp locals aera will be split by Callee saved registers and fp/ra, I think we should add a handle about this case when zeroinit the frame in CodeGen::genZeroInitFrameUsingBlockInit() to avoid clean the Callee saved registers/fp/ra on stack.
Here I think the int offset = lvaTable[lvaAsyncExecutionContextVar].GetStackOffset() + delta; , deltashould be (compCalleeRegsPushed << 3), as well as lvaMonAcquired and lvaAsyncSynchronizationContextVar.
There was a problem hiding this comment.
- Is the EnC request to enble
FEATURE_REMAP_FUNCTION? If so I think LA64 will not hit the EnC scenes temporarily. (the code under them_compiler->opts.compDbgEnCinCodeGen::genCreateAndStoreGCInfolooks improper for LA64 now. I will delete and add an assert at it.)
Yes. I see now that it is not enabled for LA64.
2. Are there any testing related to EnC? We can add EnC support for LA64 if necessary later. ( I used to run a hot reload demo at .NET6 and it's ok on LA64 at that time: https://github.com/jsuarezruiz/AvaloniaSkiaSharpFiddle ).
Currently EnC/hot reload is not enabled/supported for runtime async, so the JIT related changes to it are not tested anywhere. But eventually we will be supporting this and the JIT related changes will be important, at least for x64/arm64 etc.
cc @tommcdon on the testing question.
Thank you! If I understand correctly
lvaAsyncExecutionContextVarandlvaAsyncExecutionContextVarare temp locals, on loongarch64 (and maybe on risc-v, as similar frame layout) if place this afterlvaMonAcquired, the temp locals aera will be split byCallee saved registersandfp/ra, I think we should add a handle about this case when zeroinit the frame inCodeGen::genZeroInitFrameUsingBlockInit()to avoid clean theCallee saved registers/fp/raon stack.Here I think the
int offset = lvaTable[lvaAsyncExecutionContextVar].GetStackOffset() + delta;,deltashould be(compCalleeRegsPushed << 3), as well aslvaMonAcquiredandlvaAsyncSynchronizationContextVar.
These 3 locals (lvaMonAcquired, lvaAsyncExecutionContextVar and lvaAsyncExecutionContextVar) are special because the JIT creates them and they are long-lived, so they must be preserved on OSR and EnC transitions. To make that work they are allocated in a known place for the VM to preserve them. That happens here:
runtime/src/coreclr/vm/eetwain.cpp
Lines 188 to 209 in 0d0e129
So to support EnC in functions that use these locals you will need to take care to allow the VM to preserve those locals. But given that FEATURE_REMAP_FUNCTION is not enabled for LA64 it is perhaps not something you particularly care about.
There was a problem hiding this comment.
Thank you very much, already delete the code under if (m_compiler->opts.compDbgEnC) in CodeGen::genCreateAndStoreGCInfo() and add an assert in it for LA64.
We will implement it when enable the FEATURE_REMAP_FUNCTION and pay attention to this point.
|
Based on this PR and #124213 , the https://github.com/dotnet/runtime/tree/main/src/tests/async tests passed on JIT/R2R(use SPC-PE32+)/NativeAOT. |
|
@shushanhf @jakobbotsch @MichalStrehovsky could you please review this PR? Thanks. |
FEATURE_REMAP_FUNCTIONis not enabled on LA64.