Avoid checking HasValue in nullable comparison for non-default constants by astroC86 · Pull Request #60405 · dotnet/roslyn

astroC86 · 2022-03-26T22:30:43Z

User defined operators were not considered. (I can try to move the new logic into TrivialLiftedComparisonOperatorOptimizations so it's shared between user-defined operators and built-in operators)

Youssef1313 · 2022-03-27T06:16:35Z

Maybe we don't need anything for user-defined operators? @RikkiGibson can you confirm?

RikkiGibson · 2022-03-27T19:21:49Z

src/Compilers/CSharp/Test/Emit/CodeGen/PatternTests.cs

            compVerifier.VerifyIL("C.M2(int?)", @"
 {
-  // Code size       23 (0x17)
+  // Code size       15 (0xf)


Given the behavior of M1 versus M2 in this test, perhaps we should file an item separately for optimizing out unnecessary 'HasValue' checks in patterns.

I opened #60413

Youssef1313 · 2022-03-27T20:27:34Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_BinaryOperator.cs

            // result = (tempx.GetValueOrDefault() OP tempy.GetValueOrDefault()) &
            //          (tempx.HasValue OP tempy.HasValue)


Unrelated to the changes. @RikkiGibson Do you know why this is a bitwise AND instead of logical AND? Wouldn't that evaluate both sides and be slower? or there are other aspects I'm not aware of?

I think because we're just reading two fields here, it's faster to always do both reads than to branch on the value of the left operand.

AlekseyTs · 2022-03-28T22:45:31Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_BinaryOperator.cs

+            var canOmitHasValueCheck = yNonNull is { ConstantValue.IsDefaultValue: false } || xNonNull is { ConstantValue.IsDefaultValue: false };

            // result = (tempx.GetValueOrDefault() OP tempy.GetValueOrDefault()) &
            //          (tempx.HasValue OP tempy.HasValue)


OP

This doesn't look right for a general case.

AlekseyTs · 2022-03-28T23:02:29Z

src/Compilers/CSharp/Test/Emit2/CodeGen/CodeGenOptimizedNullableOperators.cs

+    }
+}
+";
+            CompileAndVerify(code).VerifyDiagnostics().VerifyIL("C.M", @"


CompileAndVerify(code)

I think we should verify runtime behavior as well. Execute the code and test result of M with:

null input

42

not 42

0

AlekseyTs · 2022-03-28T23:03:41Z

src/Compilers/CSharp/Test/Emit2/CodeGen/CodeGenOptimizedNullableOperators.cs

+    }
+}
+";
+            CompileAndVerify(code).VerifyDiagnostics().VerifyIL("C.M", @"


CompileAndVerify(code)

Same comment here about verifying runtime behavior. It is probably applicable to other tests in this file as well.

AlekseyTs · 2022-03-28T23:09:32Z

src/Compilers/CSharp/Test/Emit2/CodeGen/CodeGenOptimizedNullableOperators.cs

+}
+");
+        }
+    }


}

I do not see any tests with a constant on the left, but it looks like the implementation is supposed to affect scenarios like that too.

AlekseyTs · 2022-03-28T23:15:03Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_BinaryOperator.cs

-                method: null,
-                constrainedToTypeOpt: null);
+            BoundExpression binaryExpression;
+            if (canOmitHasValueCheck)


canOmitHasValueCheck

Is this logic correct for <, <=, >, >= operators? I think we need to add tests for all comparison operators that can come through here.

AlekseyTs · 2022-03-28T23:16:58Z

        BinaryOperatorKind kind,

Consider adding an assert at the beginning of the function to check what operators we expect to handle.

Refers to: src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_BinaryOperator.cs:894 in 92b4df7. [](commit_id = 92b4df7, deletion_comment = False)

AlekseyTs · 2022-03-28T23:21:08Z

Done with review pass (commit 3)

RikkiGibson · 2022-05-09T21:54:15Z

Hi @astroC86, please let us know if there's any way we can assist with the PR.

jaredpar · 2023-03-16T21:51:15Z

Closing stale PR

Avoid checking HasValue in nullable comparison for non-default constants

9152040

astroC86 requested a review from a team as a code owner March 26, 2022 22:30

ghost added Community The pull request was submitted by a contributor who is not a Microsoft employee. Area-Compilers labels Mar 26, 2022

Add tuple test

2046356

RikkiGibson self-assigned this Mar 27, 2022

Fixed Failing Tests

92b4df7

RikkiGibson reviewed Mar 27, 2022

View reviewed changes

Youssef1313 mentioned this pull request Mar 27, 2022

Optimize redundant HasValue check in patterns #60413

Open

Youssef1313 reviewed Mar 27, 2022

View reviewed changes

runfoapp bot mentioned this pull request Mar 28, 2022

Failure in CSharpCodeActions.GFUFuzzyMatchAfterRenameTrackingAndAfterGenerateType #57423

Closed

AlekseyTs reviewed Mar 28, 2022

View reviewed changes

This was referenced Mar 29, 2022

[Flaky Test] Roslyn.VisualStudio.IntegrationTests.CSharp.CSharpCodeActions.FastDoubleInvoke #60460

Closed

[Flaky integration tests] IDE version '17.2.32314.265' unexpectedly dismissed the light bulb. #60461

Closed

jaredpar closed this Mar 16, 2023

		// result = (tempx.GetValueOrDefault() OP tempy.GetValueOrDefault()) &
		// (tempx.HasValue OP tempy.HasValue)

Conversation

astroC86 commented Mar 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Youssef1313 commented Mar 27, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs Mar 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AlekseyTs commented Mar 28, 2022

Uh oh!

AlekseyTs commented Mar 28, 2022

Uh oh!

RikkiGibson commented May 9, 2022

Uh oh!

jaredpar commented Mar 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

astroC86 commented Mar 26, 2022 •

edited

Loading

AlekseyTs Mar 28, 2022 •

edited

Loading