From 466cf219cc42f0c88d29f94735f2c5bd2859624a Mon Sep 17 00:00:00 2001
From: Andy Ayers <andya@microsoft.com>
Date: Thu, 13 Apr 2017 15:17:44 -0700
Subject: [PATCH] Jit: fix unnecessary null checks in some field accesses

Morph was sometimes passing the existing MorphAddressContext down
to fgMorphField even when the field access was for a field value.
If that context contained indefinite offsets, morph would then
insert an explicit null check on the object pointer for the field
access.

Typically the field offset is small enough that this explicit check
is not needed. The implicit check done when fetching the field's value
is sufficient protection.

The fix is to have `fgMorphSmpOp` clear out the context for child
`GT_FIELD` nodes, unless the field parent is a `GT_ADDR`. Note
if there is an `op2` node the parent cannot be `GT_ADDR` so these
field children always get an empty context.

No tests added since this kicks in reasonably frequently in corelib and
elsewhere in frameworks.

Closes dotnet/coreclr#10942.


Commit migrated from https://github.com/dotnet/coreclr/commit/8936d23a89dd4508b4b5d04e93da0747ca2b7d5d
---
 src/coreclr/src/jit/morph.cpp | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/src/coreclr/src/jit/morph.cpp b/src/coreclr/src/jit/morph.cpp
index 92d5e09..dd1a59b 100644
--- a/src/coreclr/src/jit/morph.cpp
+++ b/src/coreclr/src/jit/morph.cpp
@@ -11416,6 +11416,20 @@ GenTreePtr Compiler::fgMorphSmpOp(GenTreePtr tree, MorphAddrContext* mac)
                 }
             }
 
+            // If gtOp1 is a GT_FIELD, we need to pass down the mac if
+            // its parent is GT_ADDR, since the address of the field
+            // is part of an ongoing address computation. Otherwise
+            // op1 represents the value of the field and so any address
+            // calculations it does are in a new context.
+            if ((op1->gtOper == GT_FIELD) && (tree->gtOper != GT_ADDR))
+            {
+                subMac1 = nullptr;
+
+                // The impact of this field's value to any ongoing
+                // address computation is handled below when looking
+                // at op2.
+            }
+
             tree->gtOp.gtOp1 = op1 = fgMorphTree(op1, subMac1);
 
 #if LOCAL_ASSERTION_PROP
@@ -11496,7 +11510,6 @@ GenTreePtr Compiler::fgMorphSmpOp(GenTreePtr tree, MorphAddrContext* mac)
             // (These are used to convey parent context about how addresses being calculated
             // will be used; see the specification comment for MorphAddrContext for full details.)
             // Assume it's an Ind context to start.
-            MorphAddrContext subIndMac2(MACK_Ind);
             switch (tree->gtOper)
             {
                 case GT_ADD:
@@ -11517,6 +11530,17 @@ GenTreePtr Compiler::fgMorphSmpOp(GenTreePtr tree, MorphAddrContext* mac)
                 default:
                     break;
             }
+
+            // If gtOp2 is a GT_FIELD, we must be taking its value,
+            // so it should evaluate its address in a new context.
+            if (op2->gtOper == GT_FIELD)
+            {
+                // The impact of this field's value to any ongoing
+                // address computation is handled above when looking
+                // at op1.
+                mac = nullptr;
+            }
+
             tree->gtOp.gtOp2 = op2 = fgMorphTree(op2, mac);
 
             /* Propagate the side effect flags from op2 */
-- 
2.7.4