Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check that we don't create null checks on an address that is not null. #44059

Merged

Conversation

sandreenko
Copy link
Contributor

@EgorBo in his previous Pr gave me an idea that it could be useful to check that we don't create a null check on an address that is known to be not null.

crossgen pmi
x64 windows 0 -15
arm64 windows 136 13
x64 linux -59 -32

The biggest number of diffs involves methods on arm64 windows:

Top file regressions (bytes):
         136 : System.Private.CoreLib.dasm (0.00% of base)
1 total files with Code Size differences (0 improved, 1 regressed), 267 unchanged.
Top method regressions (bytes):
          28 ( 1.40% of base) : System.Private.CoreLib.dasm - GenericEqualityComparer`1:IndexOf(ref,ValueTuple`8,int,int):int:this (7 methods)
          28 ( 1.38% of base) : System.Private.CoreLib.dasm - GenericEqualityComparer`1:LastIndexOf(ref,ValueTuple`8,int,int):int:this (7 methods)
16 total methods with Code Size differences (0 improved, 16 regressed), 195786 unchanged.

they all are in IndexOf or LastindexOf and are due to a new loop hoisting that is a size regression but a performance improvement.

@sandreenko sandreenko added enhancement Product code improvement that does NOT require public API changes/additions area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI labels Oct 30, 2020
@@ -1301,6 +1301,7 @@ inline GenTree* Compiler::gtNewIndir(var_types typ, GenTree* addr)

inline GenTree* Compiler::gtNewNullCheck(GenTree* addr, BasicBlock* basicBlock)
{
assert(fgAddrCouldBeNull(addr));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is questionable if it should be an assert or

if (!fgAddrCouldBeNull(addr))
{
  // No need for a null-check.
  return addr;
}

or something else.

Copy link
Member

Choose a reason for hiding this comment

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

it'd be weird for gtNewNullCheck to return something other than null-check 🙂

@sandreenko
Copy link
Contributor Author

PTAL @AndyAyersMS @dotnet/jit-contrib

@am11
Copy link
Member

am11 commented Oct 30, 2020

@sandreenko, would it also fix #1368, since fgAddrCouldBeNull has:

else if (addr->OperIs(GT_CNS_STR))
{
return false;
}

@EgorBo
Copy link
Member

EgorBo commented Oct 30, 2020

@sandreenko, would it also fix #1368, since fgAddrCouldBeNull has:

else if (addr->OperIs(GT_CNS_STR))
{
return false;
}

yeah that one is already fixed

@EgorBo
Copy link
Member

EgorBo commented Oct 30, 2020

@sandreenko Since you touch it, could you please address #37245 (comment) as part of this PR? 🙂

@sandreenko
Copy link
Contributor Author

@sandreenko Since you touch it, could you please address #37245 (comment) as part of this PR? 🙂

Sure, done

Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

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

I wonder if we can do something similar over in gtTryRemoveBoxUpstreamEffects -- it currently create an implicit null check, and does not check for the same "can optimize away the check" cases that the importer pattern match does.

@erozenfeld may have already tried this.

@sandreenko
Copy link
Contributor Author

I wonder if we can do something similar over in gtTryRemoveBoxUpstreamEffects -- it currently create an implicit null check, and does not check for the same "can optimize away the check" cases that the importer pattern match does.

I pushed a change with the same check for gtChangeOperToNullCheck. It should cover all cases where we create null-checks. So we will see missed-optimization opportunities as asserts if they appear.

@sandreenko sandreenko force-pushed the dontAllowNullCheckOnKnownNotNull branch from 9e13842 to 72ee330 Compare November 2, 2020 20:54
@sandreenko
Copy link
Contributor Author

I pushed a change with the same check for gtChangeOperToNullCheck. It should cover all cases where we create null-checks. So we will see missed-optimization opportunities as asserts if they appear.

Actually, I remember that I was trying to do that in the past and it did not work.
We call TransformUnusedIndirection that changes 'IND' to 'NULLCHECK' from different phases and each phase requires different transformations to delete IND and ADDR when ADDR can't be null. I have dropped the check in gtChangeOperToNullCheck, @AndyAyersMS should I open an issue to track this possible optimization in the future?

@AndyAyersMS
Copy link
Member

should I open an issue to track this possible optimization

Seems like a good idea. Would be good to note more details on why it's not as simple as it might seem.

@sandreenko sandreenko merged commit a5c194e into dotnet:master Nov 3, 2020
@sandreenko sandreenko deleted the dontAllowNullCheckOnKnownNotNull branch November 3, 2020 22:42
@ghost ghost locked as resolved and limited conversation to collaborators Dec 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI enhancement Product code improvement that does NOT require public API changes/additions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants