Review feedback

Reworded and fixed typos in some comments, inverted sense of the
null check logic.

Also added stubbed out 4.6 compat version, needed for clean desktop
builds.

Removed stale loss of type distribution data. This kind of info is
useful but should probably live elsewhere.

Author: AndyAyersMS

src/jit/flowgraph.cpp

@@ -21451,10 +21451,10 @@ void Compiler::fgAttachStructInlineeToAsg(GenTreePtr tree, GenTreePtr child, COR
 //    If the parent of the GT_RET_EXPR is a virtual call,
 //    devirtualization is attempted. This should only succeed in the
 //    successful inline case, when the inlinee's return value
-//    expression provides a better type then the declared type of the
-//    method. For failed inlines, the devirtualizer can only go by the
-//    declared return type, and any devirtualization this enables
-//    would have happened during importation.
+//    expression provides a better type than the return type of the
+//    method. Note for failed inlines, the devirtualizer can only go
+//    by the return type, and any devirtualization that type enabled
+//    would have already happened during importation.
 //
 //    If the return type is a struct type and we're on a platform
 //    where structs can be returned in multiple registers, ensure the

src/jit/importer.cpp

@@ -18407,20 +18407,27 @@ bool Compiler::IsMathIntrinsic(GenTreePtr tree)
 // Arguments:
 //     call -- the call node to examine/modify
 //     thisObj  -- the value of 'this' for the call
-//     callInfo -- info about the call from the VM
+//     callInfo -- [IN/OUT] info about the call from the VM
 //     exactContextHnd -- [OUT] updated context handle iff call devirtualized
 //
 // Notes:
 //     Virtual calls in IL will always "invoke" the base class method.
 //
 //     This transformation looks for evidence that the type of 'this'
-//     in the call is a final class or would invoke a final method, and
-//     if that and other safety checks pan out, modifies the call and
-//     the call info to create a direct call.
+//     in the call is exactly known, is a final class or would invoke
+//     a final method, and if that and other safety checks pan out,
+//     modifies the call and the call info to create a direct call.
 //
-//     This transformation is done in the importer and not in some
-//     subsequent optimization pass because we want it to be upstream
-//     of inline candidate identification.
+//     This transformation is initially done in the importer and not
+//     in some subsequent optimization pass because we want it to be
+//     upstream of inline candidate identification.
+//
+//     However, later phases may supply improved type information that
+//     can enable further devirtualization. We currently reinvoke this
+//     code after inlining, if the return value of the inlined call is
+//     the 'this obj' of a subsequent virtual call.
+
+#if COR_JIT_EE_VERSION > 460
 
 void Compiler::impDevirtualizeCall(GenTreeCall*            call,
                                    GenTreePtr              thisObj,
@@ -18454,51 +18461,18 @@ void Compiler::impDevirtualizeCall(GenTreeCall*            call,
 
     // Do we know anything about the type of the 'this'?
     //
-    // Unfortunately the jit tends to only keep track of class handles
-    // for struct types, so the type information needed here is
-    // missing for most tree nodes. We can add it back selectively,
-    // but care is needed: the presence of the class handle is often
-    // used to infer struct-ness.
-    //
-    // A rough accounting in corelib shows this breakdown for node types
-    // seen as 'this' obj in vtable calls is as follows:
-    //
-    // Node Type  Count  HasType
-    // lclVar     4256      3825
-    // field      1098      1098
-    // retExp      864
-    // intrin      318
-    // helper      294
-    // callv-vtbl  224
-    // []          123
-    // call         61
-    // box          60
-    // call-nchk    27
-    // callv-stub    5
-    //
-    // Note some lclVars are missing class handles; presumably these
-    // are locals introduced by the compiler. We might be able to add
-    // in type info for them. The table above gives a rough priority
-    // order for adding type support.
+    // Unfortunately the jit has historcally only kept track of class
+    // handles for struct types, so the type information needed here
+    // is missing for many tree nodes.
     //
     // Even when we can deduce the type, we may not be able to
     // devirtualize, but if we can't deduce the type, we can't do
     // anything.
-    //
-    // In corelib, devirtualization happens roughly 5% of the time,
-    // that is, out of 8014 virtual call sites, we devirtualize at
-    // 453. Assuming we could fill in missing types, devirtualization
-    // might ultimately kick in for around ~10% of virtual call sites.
-    //
-    // To get beyond that we'd need to do something speculative,
-    // either by adding runtime type tests into the jitted code, or by
-    // adding the ability to pitch and replace code to safely undo
-    // assumptions made about class hierarchy.
-    CORINFO_CLASS_HANDLE objClass  = nullptr;
-    GenTreePtr           obj       = thisObj->gtEffectiveVal(false);
-    const genTreeOps     objOp     = obj->OperGet();
-    bool                 nullCheck = true;
-    bool                 isExact   = false;
+    CORINFO_CLASS_HANDLE objClass     = nullptr;
+    GenTreePtr           obj          = thisObj->gtEffectiveVal(false);
+    const genTreeOps     objOp        = obj->OperGet();
+    bool                 objIsNonNull = false;
+    bool                 isExact      = false;
 
     switch (objOp)
     {
@@ -18548,8 +18522,8 @@ void Compiler::impDevirtualizeCall(GenTreeCall*            call,
 
         case GT_CNS_STR:
         {
-            objClass  = impGetStringClass();
-            nullCheck = false;
+            objClass     = impGetStringClass();
+            objIsNonNull = true;
             break;
         }
 
@@ -18689,10 +18663,8 @@ void Compiler::impDevirtualizeCall(GenTreeCall*            call,
     call->gtCallType    = CT_USER_FUNC;
 
     // Virtual calls include an implicit null check, which we may
-    // now need to make explicit. Not sure yet if we can restrict
-    // this to just a subset or need to do it for all cases, so
-    // will do it for all unless we know the object is not null.
-    if (nullCheck)
+    // now need to make explicit.
+    if (!objIsNonNull)
     {
         call->gtFlags |= GTF_CALL_NULLCHECK;
     }
@@ -18742,7 +18714,7 @@ void Compiler::impDevirtualizeCall(GenTreeCall*            call,
     callInfo->methodFlags   = derivedMethodAttribs;
     callInfo->contextHandle = MAKE_METHODCONTEXT(derivedMethod);
 
-    // Update context handle too...
+    // Update context handle.
     if ((exactContextHandle != nullptr) && (*exactContextHandle != nullptr))
     {
         *exactContextHandle = MAKE_METHODCONTEXT(derivedMethod);
@@ -18762,3 +18734,16 @@ void Compiler::impDevirtualizeCall(GenTreeCall*            call,
     }
 #endif // defined(DEBUG)
 }
+
+#else
+
+// Stubbed out implementation for 4.6 compatjit
+void Compiler::impDevirtualizeCall(GenTreeCall*            call,
+                                   GenTreePtr              thisObj,
+                                   CORINFO_CALL_INFO*      callInfo,
+                                   CORINFO_CONTEXT_HANDLE* exactContextHandle)
+{
+    return;
+}
+
+#endif // COR_JIT_EE_VERSION > 460