Skip to content

Remove the "anything + null => null" optimization - #61518

Merged
AndyAyersMS merged 2 commits into
dotnet:mainfrom
SingleAccretion:Fixing-One-More-Morph-Opt
Nov 15, 2021
Merged

Remove the "anything + null => null" optimization#61518
AndyAyersMS merged 2 commits into
dotnet:mainfrom
SingleAccretion:Fixing-One-More-Morph-Opt

Conversation

@SingleAccretion

@SingleAccretionSingleAccretion commented Nov 12, 2021

Copy link
Copy Markdown
Contributor

This optimization is only legal if:

  1. "Anything" is a sufficiently small constant itself.
  2. We are in a context where we know the address will in fact be used for an indirection.

It is the second point that is problematic - one would like to use MorphAddrContext, but it is not suitable for this purpose, as an unknown context is counted as an indirecting one. Additionally, the value of this optimization is rather low. I am guessing it was meant to support the legacy nullchecks, before GT_NULLCHECK was introduced, and had higher impact then.

So, I propose we just remove the optimization and leave the 5 small regressions across all of SPMI be.

What do the diffs look like?

Just as one would expect:

- mov byte ptr [0000H], 255- ;; bbWeight=1 PerfScore 11.75+ add rcx, -16+ mov byte ptr [rcx], 255

(Note that above is a size improvement)

+ mov esi, ecx+ add rsi, -16
mov rcx, 0xD1FFAB1E ; System.Byte
call CORINFO_HELP_NEWSFAST
; gcrRegs +[rax]
; gcr arg pop 0
- movzx rdx, byte ptr [0000H]+ movzx rdx, byte ptr [rsi]
- mov byte ptr [0000H], 0+ mov ecx, eax+ mov byte ptr [rcx], 0

Fixes#61510

Note I do not plan to do anything here with the GC-ness issue as it is separate.

This optimization is only legal if:
1) "Anything" is a sufficiently small constant itself.
2) We are in a context where we know the address will in
fact be used for an indirection.
It is the second point that is problematic - one would
like to use MorphAddrContext, but it is not suitable
for this purpose, as an unknown context is counted as
an indirecting one. Additionally, the value of this
optimization is rather low. I am guessing it was meant
to support the legacy nullchecks, before GT_NULLCHECK
was introduced, and had higher impact then.
So, just remove the optimization and leave the 5 small
regressions across all of SPMI be.
@ghostghost added community-contribution Indicates that the PR has been added by a community member area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI and removed community-contribution Indicates that the PR has been added by a community member labels Nov 12, 2021
@ghost

Copy link
Copy Markdown

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

Issue Details

This optimization is only legal if:

  1. "Anything" is a sufficiently small constant itself.
  2. We are in a context where we know the address will in fact be used for an indirection.

It is the second point that is problematic - one would like to use MorphAddrContext, but it is not suitable for this purpose, as an unknown context is counted as an indirecting one. Additionally, the value of this optimization is rather low. I am guessing it was meant to support the legacy nullchecks, before GT_NULLCHECK was introduced, and had higher impact then.

So, I propose we just remove the optimization and leave the 5 small regressions across all of SPMI be.

Author:SingleAccretion
Assignees:-
Labels:

area-CodeGen-coreclr

Milestone:-

@SingleAccretion
SingleAccretion marked this pull request as ready for review November 12, 2021 20:26
@SingleAccretion

Copy link
Copy Markdown
ContributorAuthor

@dotnet/jit-contrib

@AndyAyersMSAndyAyersMS left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this should go away.

LGTM.

@deeprobindeeprobin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@AndyAyersMS
AndyAyersMS merged commit 3d7e89c into dotnet:mainNov 15, 2021
@AndyAyersMS

Copy link
Copy Markdown
Member

Thank you!

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.

JIT optimization breaks pointer/ref arithmetic

3 participants

@SingleAccretion@AndyAyersMS@deeprobin