Repository navigation
Runtime async versions - #5
Closed
jakobbotsch wants to merge 47 commits into
Closed
jakobbotsch wants to merge 47 commits into
jakobbotsch wants to merge 47 commits into
Conversation
## Problem `runtime-diagnostics` build [1423045](https://dev.azure.com/dnceng-public/public/_build/results?buildId=1423045) and many runs since have failed `StackReferenceDumpTests.WalkStackReferences_*` on **linux_arm** R2R dumps with: ``` Microsoft.Diagnostics.DataContractReader.VirtualReadException : Failed to read System.UInt32 at 0xeff9b908. at Microsoft.Diagnostics.DataContractReader.Data.ExceptionLookupTableEntry..ctor at ...ReadyToRunJitManager.GetExceptionClauses at ...StackWalk_1.IsFilterFunclet ``` 5 of 8 recent builds (1416195 - 1423045) failed across 6 CdacXPlatDumpTest legs. ## Root cause Since dotnet#128154, `IExecutionManager.GetStartAddress` returns a `TargetPointer` with the ARM32 thumb bit stripped (matching native `TADDR` semantics, returned by `RUNTIME_FUNCTION__BeginAddress` in `clrnt.h`). `GetMethodInfo` was updated to honor that, but `GetFuncletStartAddress` was missed: ```csharp // ExecutionManagerCore.cs (before fix) return range.Data.RangeBegin + runtimeFunction.BeginAddress; // thumb bit preserved on ARM32 ``` ### Consequence on ARM32 - `GetStartAddress` returns `0x........0` (clean) - `GetFuncletStartAddress` returns `0x........1` (thumb bit set) - `IsFunclet` (`return startAddress != GetFuncletStartAddress(...)`) returns **true** for every R2R method - `IsFilterFunclet` then calls `GetExceptionClauses` for every non-funclet, which tries to binary-search the R2R `ExceptionInfo` lookup table whose data is not paged into the minidump -> `VirtualReadException` at `0xeff9b908` arm64/x64 do not have a thumb bit, so the comparison still matched there. ## Fix Strip the thumb bit at the call site by running the `RangeBegin + BeginAddress` sum through `CodePointerUtils.AddressFromCodePointer` -- the cdac analog of native `ThumbCodeToDataPointer` used by `RUNTIME_FUNCTION__BeginAddress`. Fixes dotnet#128292. ## Validation - Built cdac Contracts: clean. - `dotnet test` filtered to `ExecutionManager`: 285/285 pass. - runtime-diagnostics CI will validate end-to-end behavior on ARM32 dumps. > [!NOTE] > This PR was authored with assistance from GitHub Copilot. --------- Co-authored-by: Max Charlamb <maxcharlamb@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…dotnet#128521) The VEX 2-byte encoding allows `REX.R` to be on or off while it requires that `REX.B` be off. This means that `op1` can encode `[XMM0, XMM15]` while `op2` can only encode `[XMM0, XMM7]. Thus, when we have a commutative instruction we want to check for this scenario and swap the operands. If both are not "extended" then we'll get the 2-byte encoding regardless, while if both are extended we'll require the 3-byte encoding. This doesn't handle some of the comparison cases that also qualify but which require swapping the comparison that is being done. ```diff - vpand xmm3, xmm3, xmm8 - ;; size=5 + vpand xmm3, xmm8, xmm3 + ;; size=4 ``` --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
The JIT will sometimes commit to fully-interruptible GC too early. Wait until we've had a second chance to remove EH regions.
…otnet#128328) Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: MihaZupan <25307628+MihaZupan@users.noreply.github.com>
Two of the three custom agent files under `.github/agents/` were missing the `.agent.md` suffix and were therefore silently not loaded by the Copilot CLI / VS Code / Claude Code: | Before | After | |--------|-------| | `.github/agents/extensions-reviewer.md` | `.github/agents/extensions-reviewer.agent.md` | | `.github/agents/system-net-review.md` | `.github/agents/system-net-review.agent.md` | Per the [Copilot CLI plugin reference](https://docs.github.com/en/copilot/reference/cli-plugin-reference#component-path-fields), agents are discovered as `.agent.md` files inside the agents directory. Without that suffix, the file is not registered as an agent. The third agent file, `agentic-workflows.agent.md`, was already named correctly and is unchanged. ## Why this matters This fixes broken `@`-mentions in two skills that delegate to these agents: - `.github/skills/extensions-review/SKILL.md` says *"For full code review, delegates to the @extensions-reviewer agent"* - `.github/skills/system-net-review/SKILL.md` says *"For full code review, delegates to the @system-net-review agent"* With the agents not being loaded, those `@`-mentions had nothing to resolve to. After this rename both delegations will work as documented. ## What's in this PR Pure file renames preserved via `git mv` (similarity 100%). No content changes. No other files reference these agents by filename — only by `@`-mention via the agent's `name` field, which is unaffected. ``` .github/agents/{extensions-reviewer.md => extensions-reviewer.agent.md} | 0 .github/agents/{system-net-review.md => system-net-review.agent.md} | 0 2 files changed, 0 insertions(+), 0 deletions(-) ```
Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: adamsitnik <6011991+adamsitnik@users.noreply.github.com>
This adds an optimization that tail merges suspension points when predecessors of joins end with async calls.
This PR makes `O1K_LCLVAR` local assertion prop only, it's fully
replaced with `O1K_VN` in global assertion prop now.
Interestingly, it catches a few constant-props that are useful to unroll
more SequenceEqual, e.g:
```cs
static readonly string Temp = Environment.GetEnvironmentVariable("TEMP");
static bool Test(string str) => str == Temp;
```
**Codegen before this PR:**
```asm
; Assembly listing for method Prog:Test(System.String):bool (Tier1)
sub rsp, 40
mov rdx, 0x1BF978001C0 ; const ptr
mov rdx, gword ptr [rdx]
cmp rcx, rdx
je SHORT G_M47511_IG05
test rcx, rcx
je SHORT G_M47511_IG06
mov r8d, dword ptr [rcx+0x08]
cmp r8d, 33
jne SHORT G_M47511_IG06
add rcx, 12
add rdx, 12
add r8d, r8d
call [System.SpanHelpers:SequenceEqual(byref,byref,nuint):bool]
G_M47511_IG03:
nop
add rsp, 40
ret
G_M47511_IG05:
mov eax, 1
jmp SHORT G_M47511_IG03
G_M47511_IG06:
xor eax, eax
jmp SHORT G_M47511_IG03
; Total bytes of code 71, prolog size 4, PerfScore 13.50
```
**With this PR:**
```asm
; Assembly listing for method Prog:Test(System.String):bool (Tier1)
mov rax, 0x2A43C8001C0 ; const ptr
mov rax, gword ptr [rax]
cmp rcx, rax
je SHORT G_M47511_IG04
test rcx, rcx
je SHORT G_M47511_IG05
mov edx, dword ptr [rcx+0x08]
cmp edx, 33
jne SHORT G_M47511_IG05
add rcx, 12
add rax, 12
vmovups zmm0, zmmword ptr [rcx]
vmovups zmm1, zmmword ptr [rcx+0x02]
vpxord zmm1, zmm1, zmmword ptr [rax+0x02]
vpternlogq zmm0, zmm1, zmmword ptr [rax], -34
vptestmq k1, zmm0, zmm0
kortestb k1, k1
sete al
movzx rax, al
G_M47511_IG03:
vzeroupper
ret
G_M47511_IG04:
mov eax, 1
jmp SHORT G_M47511_IG03
G_M47511_IG05:
xor eax, eax
jmp SHORT G_M47511_IG03
; Total bytes of code 103, prolog size 0, PerfScore 33.25
```
^ unfortunately, PerfScore (and size) treat it as a pure regression,
which is not true. We probably should improve our PerfScore for calls
(e.g. actual PerfScore of the call's impl?)
…28428) We preallocate this array, with IL code size capacity, but we don't always add maps to it. The runtime has an assert that the array should be null if the size is 0. The map can be empty with simple methods like: ``` IL_0000 newobj , sp 0, IL_0005 ret , sp 1, O System.Text.Json.Utf8JsonWriter ``` Regressed after dotnet#127469 where we reduce the amount of map entries to locations with empty stack. Fixes assertions like `FAILED: (iOffsetMapping == 0) == (pOffsetMapping == 0)` Fixes dotnet#127663
uint GetStackParameterSize(CodeBlockHandle codeInfoHandle) => throw new NotImplementedException(); --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: rcj1 <77995559+rcj1@users.noreply.github.com> Co-authored-by: Rachel Jarvi <rachel.jarvi@gmail.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Ensure the new store fully overlaps the old. Fixes dotnet#128566.
Extend the per-funclet unwind info to record the length of the virtual IP range. The lowest Virtual IP for a funclet can be found by summing the lengths of all the prior funclets. The unwind info previously just recorded the size of the fixed part of the frame. Virtual IP ranges for funclets (and main method) are disjoint. Data is encoded as ULEB128.
Generalize loop cloning a bit: * allow more statements between increment and test * allow cloning conditions to establish a zero trip test if one is missing. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…dotnet#128235) Previously, crossgen2 would emit addends on relocations to encode the thumb bit in arm32. This PR unifies behavior between crossgen2 and ilc to emit the symbol definitions of "PCode" symbols to include the thumb bit. Adds an IPCodeSymbolNode interface to mark which symbols should have the thumb bit set in their symbol definition. The JIT now assumes all compilations that use Relocations will put the thumb bit in the symbol definitions and won't set it in those compilations. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…otnet#128528) There was a subtle behavior difference between runtime async and async1 continuations when multiple of them were registered on a task. For async1, `Task.RunContinuations` will run async1 continuations except the first one in parallel, and then synchronously invoke the first one. However, for runtime async we were using `ITaskCompletionAction` in this case. This has different behavior: when multiple of these are present they are all invoked sequentially. See the test introduced in this PR (which was based on [this](https://github.com/dotnet/runtime/blob/4e4d02afbe0b8144c8b3c8f1fdf25cacf5663b3c/src/libraries/System.Runtime/tests/System.Threading.Tasks.Tests/System.Runtime.CompilerServices/TaskAwaiterTests.cs#L182-L186) test). This PR fixes the problem by making `TaskContinuation` the continuation object registered on `Task` instances, instead of `RuntimeAsyncTask`. To that end rename the class to `RuntimeAsyncTaskContinuation` and teach `Task.RunContinuations` about this kind of continuation. It differs from `ITaskCompletionAction` in that all such instances are run in parallel, with only the first instance being invoked synchronously. Then consistently use this object as the continuation object for task awaits. This is done for both `AsyncHelpers.TransparentAwait` and `AsyncHelpers.Await`, so all `Await` calls are changed to use similar task-caching optimization as we were doing for value task awaits before. In addition optimize them in several ways: - Teach the JIT to allow inlining of async functions when all awaits are tail awaits (using `AsyncHelpers.TailAwait`) - Rewrite all await helpers in the tail await shape to allow inlining of them - Optimize all await paths to use hand rolled continuations with caching and that avoids additional async1 continuation allocations
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.