Skip to content

JIT: arm64 osr funclet frame probing - #70916

Merged
AndyAyersMS merged 2 commits into
dotnet:mainfrom
AndyAyersMS:Fix70263
Jun 23, 2022
Merged

JIT: arm64 osr funclet frame probing#70916
AndyAyersMS merged 2 commits into
dotnet:mainfrom
AndyAyersMS:Fix70263

Conversation

@AndyAyersMS

@AndyAyersMSAndyAyersMS commented Jun 17, 2022

Copy link
Copy Markdown
Member

We currently need to pad OSR funclet frames with the full size of the
Tier0 frame, and this padding sits above the register save area. For
large Tier0 frames we thus need to probe the stack in the OSR prolog
for type1 and type3 frames, before doing the initial SP adjustment and
save.

Longer term the plan is to revise arm64 OSR so we will not need to
do this padding; at that point we can remove this extra probing.

Fixes#70263.

@ghostghost added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jun 17, 2022
@ghost

Copy link
Copy Markdown

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

We currently need to pad OSR funclet frames with the full size of the
Tier0 frame, and this paddign sits above the register save area. For
large Tier0 frames we thus need to probe the stack in the OSR prolog
for type1 and type3 frames, before doing the initial SP adjustment and
save.

Longer term the plan is to revise arm64 OSR so we will not need to
do this padding; at that point we can remove this extra probing.

Fixes #70263.

Author:AndyAyersMS
Assignees:-
Labels:

area-CodeGen-coreclr

Milestone:-

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

@BruceForstall PTAL
cc @dotnet/jit-contrib

@BruceForstall
BruceForstall self-requested a review June 17, 2022 22:21
Comment threadsrc/coreclr/jit/codegenarm64.cpp Outdated
Comment threadsrc/coreclr/jit/codegenarm64.cpp Outdated
@BruceForstall

Copy link
Copy Markdown
Contributor

Can you show what the funclet prolog from #70263 (comment) looks like with this change?

@BruceForstall

Copy link
Copy Markdown
Contributor

What about the genStackPointerAdjustment(genFuncletInfo.fiSpDelta1, REG_SCRATCH, nullptr, /* reportUnwindData */ true); done for frame type 5? What range is genFuncletInfo.fiSpDelta1 in that case? (can there be asserts added that indicate the maximum range?)

@jakobbotsch

Copy link
Copy Markdown
Member

"Frame type 5" is already quite broken, see #66089.

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

Can you show what the funclet prolog from #70263 (comment) looks like with this change?

Old/new prologs

mov x9, #0x9ce0subsp,sp, x9stp fp, lr,[sp]stp x19, x20,[sp,#32]mov x2, #0x9ce0add x3, fp, x2str x3,[sp,#24]
movn x9, #0xfffmovn x1, #0x9cdfldr wzr,[sp, x9]sub x9, x9, #1,LSL #12cmp x1, x9bls pc-16 (-4 instructions)mov x9, #0x9ce0subsp,sp, x9stp fp, lr,[sp]stp x19, x20,[sp,#32]mov x2, #0x9ce0add x3, fp, x2str x3,[sp,#24]

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr superpmi-diffs

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-coreclr superpmi-replay

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

1 similar comment
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

caught up in the guid change

@AndyAyersMS

AndyAyersMS commented Jun 18, 2022

Copy link
Copy Markdown
MemberAuthor

Libraries failure looks like a gc hole perhaps:

Assert failure(PID 5564 [0x000015bc], Thread: 708 [0x02c4]): m_alignpad == 0

This is on x64 -- this PR only changes arm64 codegen, so expect this is unrelated.

@jkotas

Copy link
Copy Markdown
Member

Assert failure(PID 5564 [0x000015bc], Thread: 708 [0x02c4]): m_alignpad == 0

#70231

@jakobbotsch

Copy link
Copy Markdown
Member

/azp run runtime-coreclr superpmi-diffs, runtime-coreclr superpmi-replay

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 2 pipeline(s).

@BruceForstall

Copy link
Copy Markdown
Contributor

You should rebase after #70922, and I think you need the same change for fiFrameType=5.

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

You should rebase after #70922, and I think you need the same change for fiFrameType=5.

Ok. You don't mind that I am repurposing genAllocLclFrame like this? It is (more or less) allocating the "tier0" frame equivalent in the funclet.

I really need to revise arm64 to not need this crazy funclet padding, but it feels a bit risky to try doing that now.

@BruceForstall

Copy link
Copy Markdown
Contributor

You don't mind that I am repurposing genAllocLclFrame like this? It is (more or less) allocating the "tier0" frame equivalent in the funclet.

It seems ok, since genAllocLclFrame on arm64 only does frame probing (the name is a misnomer for arm64). So as long as it's probing the right amount, for OSR, it makes sense.

We currently need to pad OSR funclet frames with the full size of the
Tier0 frame, and this paddign sits above the register save area. For
large Tier0 frames we thus need to probe the stack in the OSR prolog
for type1 and type3 frames, before doing the initial SP adjustment and
save.
Longer term the plan is to revise arm64 OSR so we will not need to
do this padding; at that point we can remove this extra probing.
Fixesdotnet#70263.
@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

/azp run runtime-jit-experimental

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

Failure looks like #69474

@AndyAyersMS

Copy link
Copy Markdown
MemberAuthor

Jit-experimental was clean other than an OSX timeout.

Other parts of this pipeline run the Github_21990 test case under OSR stress, and it's been consistently failing there on arm64 for a long time.

@AndyAyersMS
AndyAyersMS merged commit 95d64e8 into dotnet:mainJun 23, 2022
@AndyAyersMSAndyAyersMS mentioned this pull request Jun 24, 2022
72 tasks
@ghostghost locked as resolved and limited conversation to collaborators Jul 23, 2022
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-CodeGen-coreclrCLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Test failure: JIT\Regression\JitBlue\GitHub_21990\GitHub_21990\GitHub_21990.cmd

4 participants

@AndyAyersMS@BruceForstall@jakobbotsch@jkotas