Fix impBoxPatternMatch

[SingleAccretion]

For the BOX;BR[COND] case, the issue was that the code assumed that we only need to hold onto the original address tree in case a null check is needed, which is not the case -- the address could be both non-null and itself side-effecting (e. g. ADDR(INDEX)).

Diffs.

[ghost]

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

Issue Details

---

The issue was that the code assumes that we only need to hold onto the original address tree in case a null check is needed, which is not the case -- the address could be both non-null and itself side-effecting (e. g. ADDR(INDEX)).

The narrow fix would have been to disallow side-effecting addresses, but I went for the more general improvement of spilling the side effecting tree as unused value instead, because it makes the code simpler, and will avoid diffs for one of the future changes.

So, we're expecting diffs - some size regressions that are actually PerfScore improvements due to new CSEs.

Author:	SingleAccretion
Assignees:	-
Labels:	area-CodeGen-coreclr, community-contribution
Milestone:	-

[SingleAccretion]

@dotnet/jit-contrib I would like to request a re-run of the SPMI diffs for this change. It failed to download the Jits from the rolling build it seems:

Downloading:

  https://clrjit2.blob.core.windows.net/jitrollingbuild/builds/4198a8af7caa3f9f28db68a90ef4b318f7edb50c/windows/x64/Checked/clrjit.dll

  https://clrjit2.blob.core.windows.net/jitrollingbuild/builds/4198a8af7caa3f9f28db68a90ef4b318f7edb50c/windows/x64/Checked/clrjit_unix_x64_x64.dll

  https://clrjit2.blob.core.windows.net/jitrollingbuild/builds/4198a8af7caa3f9f28db68a90ef4b318f7edb50c/windows/x64/Checked/clrjit_win_x64_x64.dll

  https://clrjit2.blob.core.windows.net/jitrollingbuild/builds/4198a8af7caa3f9f28db68a90ef4b318f7edb50c/windows/x64/Checked/clrjit_win_x86_x64.dll

Download: https://clrjit2.blob.core.windows.net/jitrollingbuild/builds/4198a8af7caa3f9f28db68a90ef4b318f7edb50c/windows/x64/Checked/clrjit.dll -> D:\a\_work\1\s\payload\base\clrjit.dll

Download: https://clrjit2.blob.core.windows.net/jitrollingbuild/builds/4198a8af7caa3f9f28db68a90ef4b318f7edb50c/windows/x64/Checked/clrjit_universal_arm64_x64.dll -> D:\a\_work\1\s\payload\base\clrjit_universal_arm64_x64.dll

Download: https://clrjit2.blob.core.windows.net/jitrollingbuild/builds/4198a8af7caa3f9f28db68a90ef4b318f7edb50c/windows/x64/Checked/clrjit_universal_arm_x64.dll -> D:\a\_work\1\s\payload\base\clrjit_universal_arm_x64.dll

Download: https://clrjit2.blob.core.windows.net/jitrollingbuild/builds/4198a8af7caa3f9f28db68a90ef4b318f7edb50c/windows/x64/Checked/clrjit_unix_x64_x64.dll -> D:\a\_work\1\s\payload\base\clrjit_unix_x64_x64.dll

Download: https://clrjit2.blob.core.windows.net/jitrollingbuild/builds/4198a8af7caa3f9f28db68a90ef4b318f7edb50c/windows/x64/Checked/clrjit_win_x64_x64.dll -> D:\a\_work\1\s\payload\base\clrjit_win_x64_x64.dll

...

ConnectionResetError: [WinError 10054] An existing connection was forcibly closed by the remote host

(I've noticed this started happening quite often in the last few days)

[jakobbotsch]

/azp run runtime-coreclr superpmi-diffs

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[jakobbotsch]

Any reason not to spill for the other patterns as well? Then we could remove the the special handling for byref-like types before this function is called.

[SingleAccretion]

Any reason not to spill for the other patterns as well?

Nothing that I would think of I suppose, I'll make the changes.

[SingleAccretion]

I have scaled back the change to the narrow correctness fix, as tying together the many ways of how unused values can be transformed while avoiding regressions proved a little fruitless.

Details

There are 4 ways an unused tree can be transformed:

1. Struct, into an address.
2. Struct load, into a nullcheck.
3. Primitive, left alone.
4. Primitive, wrapped in a COMMA.

It so happens that for different trees the different strategies produce different results CQ-wise, due to the differences in how friendly a given shape is to CSE and morph's dead code removal. This area is full of quirky behavior, so I decided to leave it be for now.

[SingleAccretion]

The Installer Build and Test coreclr windows_x64 Release timeout looks unlikely to be related; will request a re-run.

@dotnet/jit-contrib

[jakobbotsch]

That one is #69114 so not related.