JIT: Sparse conditional propagation in value numbering #94563

jakobbotsch · 2023-11-09T13:20:59Z

Utilize the fact that VN is able to decide whether many non-trivial branches will be taken or not taken to prove basic blocks dynamically unreachable. Take this into account when building PHIs.

Fix #94559

There are some additional improvements to make:

I'm not sure if we see statically unreachable blocks, but if we do, we should try to handle them similarly. Currently it seems like we pessimize PHIs if these are present.
We should fully skip VN'ing the blocks that are proven unreachable, but downstream phases currently are not prepared to handle that. We should maybe eliminate the proven unreachable blocks as part of VN to make sure downstream phases see a consistent view. (This is also necessary to make VN correct on its own when it is utilizing liberal VNs to prove the reachability.)
~~I couldn't figure out how to look up the predecessor corresponding to a memory PHI arg, so currently we do not do this for memory PHIs.~~ Adding the pred blocks for these and making use of them has almost no diffs, so doesn't seem worth it.

Codegen of the example from #94559 before:

       mov      rcx, 0xD1FFAB1E      ; ubyte[]
       mov      edx, 128
       call     CORINFO_HELP_NEWARR_1_VC
       add      rax, 16
       mov      ecx, 128
       xor      rdx, rdx
       test     ecx, ecx
       cmovne   rdx, rax
       mov      bword ptr [rsp+0x20], rdx
       mov      rcx, rdx
       call     [Program:DoSomething(ulong)]
       xor      eax, eax
       mov      bword ptr [rsp+0x20], rax

Codegen after:

       mov      rcx, 0xD1FFAB1E      ; ubyte[]
       mov      edx, 128
       call     CORINFO_HELP_NEWARR_1_VC
       add      rax, 16
       mov      rcx, rax
       mov      bword ptr [rsp+0x20], rcx
       call     [Program:DoSomething(ulong)]
       xor      eax, eax
       mov      bword ptr [rsp+0x20], rax

Utilize the fact that VN is able to decide whether many non-trivial branches will be taken or not taken to prove basic blocks dynamically unreachable. Take this into account when building PHIs. Fix dotnet#94559

ghost · 2023-11-09T13:21:10Z

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

Issue Details

Utilize the fact that VN is able to decide whether many non-trivial branches will be taken or not taken to prove basic blocks dynamically unreachable. Take this into account when building PHIs.

Fix #94559

There are some additional improvements to make:

I'm not sure if we see statically unreachable blocks, but if we do, we should try to handle them similarly. Currently it seems like we pessimize PHIs if these are present.
We should fully skip VN'ing the blocks that are proven unreachable, but downstream phases currently are not prepared to handle that. We should maybe eliminate the proven unreachable blocks as part of VN to make sure downstream phases see a consistent view.

Author:	jakobbotsch
Assignees:	-
Labels:	`area-CodeGen-coreclr`
Milestone:	-

jakobbotsch · 2023-11-09T13:35:32Z

It would also be possible to add some of RBO's smarts in here to make it even stronger, of course at a TP cost.

jakobbotsch · 2023-11-09T15:13:48Z

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress

azure-pipelines · 2023-11-09T15:14:08Z

Azure Pipelines successfully started running 2 pipeline(s).

jakobbotsch · 2023-11-09T15:16:40Z

/azp run Fuzzlyn

azure-pipelines · 2023-11-09T15:16:51Z

Azure Pipelines successfully started running 1 pipeline(s).

The assert that the phi loop found a phi could be hit in cases where a block had a statically unreachable pred. In that case SSA may not have added any PHI arg for that pred, and if we then proved all other preds unreachable, we would end up with no VN for a phi. Fix it by using the value of the last phi arg in those cases.

jakobbotsch · 2023-11-09T21:49:36Z

/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress, Fuzzlyn

azure-pipelines · 2023-11-09T21:49:53Z

Azure Pipelines successfully started running 3 pipeline(s).

jakobbotsch · 2023-11-10T09:31:14Z

cc @dotnet/jit-contrib PTAL @AndyAyersMS

Diffs. Mostly dominated by tests, in the real world I think the PHI disambiguation that RBO does handles most cases already. However, doing it this way in VN is naturally more precise. I want to try moving some of RBO's smarts to VN (at least the simple dominator checks).

TP regressions weren't as bad as I expected, so I didn't bother to do more work to optimize the TP. I also ended up not doing the work to switch to a precomputed RPO. I still want to do that, but as a follow-up.

AndyAyersMS · 2023-11-10T15:38:00Z

src/coreclr/jit/valuenum.cpp

+                    continue;
+                }
+
+                JITDUMP("  ..but it looks like all preds are unreachable, so we are using it anyway\n");


Maybe add a comment that since we're quite sure this block is unreachable it's ok to use a somewhat arbitrary VN for the PHI? Can add it later if you're going to tweak nearby code again soon (for rbo say).

Will do that in the follow-up.

AndyAyersMS

Changes LGTM.

If you did switch over to the new RPO you could identify statically unreachable blocks. But maybe these are rare. At any rate rather than tolerate them here we probably should get rid of them before (or just after) we build SSA; it has identified these blocks too.

jakobbotsch · 2023-11-10T23:01:38Z

If you did switch over to the new RPO you could identify statically unreachable blocks.

Yeah, I'll make use of that in the follow-up.

JIT: Sparse conditional propagation in value numbering

bcbaaf0

Utilize the fact that VN is able to decide whether many non-trivial branches will be taken or not taken to prove basic blocks dynamically unreachable. Take this into account when building PHIs. Fix dotnet#94559

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Nov 9, 2023

ghost assigned jakobbotsch Nov 9, 2023

This was referenced Nov 9, 2023

CI error: System.Net.Quic.QuicException: The connection timed out from inactivity #91757

Closed

AV in LibraryImport tests #93155

Closed

jakobbotsch added 4 commits November 9, 2023 21:45

Go back to liberal VNs

84d764c

Run jit-format

bc51c3c

Nit

bde2d36

build-analysis bot mentioned this pull request Nov 10, 2023

Test_EventSource_EtwManifestGeneration* tests failing in CI #48798

Closed

jakobbotsch marked this pull request as ready for review November 10, 2023 09:31

jakobbotsch requested a review from AndyAyersMS November 10, 2023 09:31

AndyAyersMS reviewed Nov 10, 2023

View reviewed changes

AndyAyersMS approved these changes Nov 10, 2023

View reviewed changes

jakobbotsch merged commit cc78af7 into dotnet:main Nov 10, 2023
192 of 196 checks passed

jakobbotsch deleted the vn-sparse-conditional-propagation branch November 10, 2023 23:02

jakobbotsch mentioned this pull request Nov 27, 2023

Bounds check not eliminated with enumerator #78000

Closed

github-actions bot locked and limited conversation to collaborators Dec 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JIT: Sparse conditional propagation in value numbering #94563

JIT: Sparse conditional propagation in value numbering #94563

jakobbotsch commented Nov 9, 2023 •

edited

Loading

ghost commented Nov 9, 2023

jakobbotsch commented Nov 9, 2023

jakobbotsch commented Nov 9, 2023

azure-pipelines bot commented Nov 9, 2023

jakobbotsch commented Nov 9, 2023

azure-pipelines bot commented Nov 9, 2023

jakobbotsch commented Nov 9, 2023

azure-pipelines bot commented Nov 9, 2023

jakobbotsch commented Nov 10, 2023

AndyAyersMS Nov 10, 2023

jakobbotsch Nov 10, 2023

AndyAyersMS left a comment

jakobbotsch commented Nov 10, 2023

JIT: Sparse conditional propagation in value numbering #94563

JIT: Sparse conditional propagation in value numbering #94563

Conversation

jakobbotsch commented Nov 9, 2023 • edited Loading

ghost commented Nov 9, 2023

jakobbotsch commented Nov 9, 2023

jakobbotsch commented Nov 9, 2023

azure-pipelines bot commented Nov 9, 2023

jakobbotsch commented Nov 9, 2023

azure-pipelines bot commented Nov 9, 2023

jakobbotsch commented Nov 9, 2023

azure-pipelines bot commented Nov 9, 2023

jakobbotsch commented Nov 10, 2023

AndyAyersMS Nov 10, 2023

Choose a reason for hiding this comment

jakobbotsch Nov 10, 2023

Choose a reason for hiding this comment

AndyAyersMS left a comment

Choose a reason for hiding this comment

jakobbotsch commented Nov 10, 2023

jakobbotsch commented Nov 9, 2023 •

edited

Loading