Address CR feedback.

Author: pgavlin

src/inc/corjit.h

@@ -85,7 +85,7 @@ enum CorJitFlag
     CORJIT_FLG_GCPOLL_CALLS        = 0x00000040, // Emit calls to JIT_POLLGC for thread suspension.
     CORJIT_FLG_MCJIT_BACKGROUND    = 0x00000080, // Calling from multicore JIT background thread, do not call JitComplete
 
-#ifdef FEATURE_LEGACYNETCF
+#if defined(FEATURE_LEGACYNETCF)
 
     CORJIT_FLG_NETCF_QUIRKS        = 0x00000100, // Mimic .NetCF JIT's quirks for generated code (currently just inlining heuristics)
 
@@ -121,25 +121,33 @@ enum CorJitFlag
 
 #endif // !defined(_TARGET_X86_) && !defined(_TARGET_AMD64_)
 
-#ifdef MDIL
+#if defined(MDIL)
+
     CORJIT_FLG_MDIL                = 0x00004000, // Generate MDIL code instead of machine code
 
     // Safe to overlap with CORJIT_FLG_MAKEFINALCODE below. Not used by the JIT, used internally by NGen only.
     CORJIT_FLG_MINIMAL_MDIL        = 0x00008000, // Generate MDIL code suitable for use to bind other assemblies.
 
     // Safe to overlap with CORJIT_FLG_READYTORUN below. Not used by the JIT, used internally by NGen only.
     CORJIT_FLG_NO_MDIL             = 0x00010000, // Generate an MDIL section but no code or CTL. Not used by the JIT, used internally by NGen only.
-#else // MDIL
+
+#else // defined(MDIL)
+
     CORJIT_FLG_CFI_UNWIND          = 0x00004000, // Emit CFI unwind info
-#endif // MDIL
 
 #if defined(FEATURE_INTERPRETER)
+
     CORJIT_FLG_MAKEFINALCODE       = 0x00008000, // Use the final code generator, i.e., not the interpreter.
-#endif // FEATURE_INTERPRETER
 
-#ifdef FEATURE_READYTORUN_COMPILER
+#endif // defined(FEATURE_INTERPRETER)
+
+#if defined(FEATURE_READYTORUN_COMPILER)
+
     CORJIT_FLG_READYTORUN          = 0x00010000, // Use version-resilient code generation
-#endif
+
+#endif // defined(FEATURE_READYTORUN_COMPILER)
+
+#endif // !defined(MDIL)
 
     CORJIT_FLG_PROF_ENTERLEAVE     = 0x00020000, // Instrument prologues/epilogues
     CORJIT_FLG_PROF_REJIT_NOPS     = 0x00040000, // Insert NOPs to ensure code is re-jitable

src/jit/codegencommon.cpp

@@ -8522,7 +8522,7 @@ void                CodeGen::genFnProlog()
     {
         excludeMask |= RBM_PINVOKE_FRAME;
 
-        assert(!compiler->opts.ShouldUsePInvokeHelpers() || compiler->info.compLvFrameListRoot == BAD_VAR_NUM);
+        assert((!compiler->opts.ShouldUsePInvokeHelpers()) || (compiler->info.compLvFrameListRoot == BAD_VAR_NUM));
         if (!compiler->opts.ShouldUsePInvokeHelpers())
         {
             noway_assert(compiler->info.compLvFrameListRoot < compiler->lvaCount);

src/jit/lower.cpp

@@ -2477,7 +2477,10 @@ void Lowering::InsertPInvokeMethodProlog()
 
     if (comp->opts.ShouldUsePInvokeHelpers())
     {
-        // Initialize the P/Invoke frame by calling CORINFO_HELP_INIT_PINVOKE_FRAME
+        // Initialize the P/Invoke frame by calling CORINFO_HELP_INIT_PINVOKE_FRAME:
+        //
+        // OpaqueFrame opaqueFrame;
+        // CORINFO_HELP_INIT_PINVOKE_FRAME(&opaqueFrame);
 
         GenTree* frameAddr = new(comp, GT_LCL_VAR_ADDR)
             GenTreeLclVar(GT_LCL_VAR_ADDR, TYP_BYREF, comp->lvaInlinedPInvokeFrameVar, BAD_IL_OFFSET);
@@ -2845,6 +2848,55 @@ GenTree* Lowering::LowerNonvirtPinvokeCall(GenTreeCall* call)
     //------------------------------------------------------
     // Non-virtual/Indirect calls: PInvoke calls.
 
+    // PInvoke lowering varies depending on the flags passed in by the EE. By default,
+    // GC transitions are generated inline; if CORJIT_FLG2_USE_PINVOKE_HELPERS is specified,
+    // GC transitions are instead performed using helper calls. Examples of each case are given
+    // below. Note that the data structure that is used to store information about a call frame
+    // containing any P/Invoke calls is initialized in the method prolog (see
+    // InsertPInvokeMethod{Prolog,Epilog} for details).
+    //
+    // Inline transitions:
+    //     InlinedCallFrame inlinedCallFrame;
+    //
+    //     ...
+    //
+    //     // Set up frame information
+    //     inlinedCallFrame.callTarget = methodHandle;
+    //     inlinedCallFrame.callSiteTracker = SP; (x86 only)
+    //     inlinedCallFrame.callSiteReturnAddress = &label; (the address of the instruction immediately following the call)
+    //     thread->m_pFrame = &inlinedCallFrame; (non-IL-stub only)
+    //
+    //     // Switch the thread's GC mode to preemptive mode
+    //     thread->m_fPreemptiveGCDisabled = 0;
+    //
+    //     // Call the unmanged method
+    //     target();
+    //
+    //     // Switch the thread's GC mode back to cooperative mode
+    //     thread->m_fPreemptiveGCDisabled = 1;
+    //
+    //     // Rendezvous with a running collection if necessary
+    //     if (g_TrapReturningThreads)
+    //         RareDisablePreemptiveGC();
+    //
+    // Transistions using helpers:
+    //
+    //     OpaqueFrame opaqueFrame;
+    //
+    //     ...
+    //
+    //     // Call the JIT_PINVOKE_BEGIN helper
+    //     JIT_PINVOKE_BEGIN(&opaqueFrame);
+    //
+    //     // Call the unmanaged method
+    //     target();
+    //
+    //     // Call the JIT_PINVOKE_END helper
+    //     JIT_PINVOKE_END(&opaqueFrame);
+    //
+    // Note that the JIT_PINVOKE_{BEGIN.END} helpers currently use the default calling convention for the target platform.
+    // They may be changed in the near future s.t. they preserve all register values.
+
     GenTree* result = nullptr;
     void* addr = nullptr;
 

src/jit/regalloc.cpp

@@ -6438,21 +6438,18 @@ void               Compiler::rpPredictRegUse()
         // it must not be in a register trashed by the callee
         if (info.compCallUnmanaged != 0)
         {
-            assert(!opts.ShouldUsePInvokeHelpers() || info.compLvFrameListRoot == BAD_VAR_NUM);
-            if (!opts.ShouldUsePInvokeHelpers())
-            {
-                noway_assert(info.compLvFrameListRoot < lvaCount);
+            assert(!opts.ShouldUsePInvokeHelpers());
+            noway_assert(info.compLvFrameListRoot < lvaCount);
 
-                LclVarDsc *     pinvokeVarDsc = &lvaTable[info.compLvFrameListRoot];
+            LclVarDsc *     pinvokeVarDsc = &lvaTable[info.compLvFrameListRoot];
 
-                if (pinvokeVarDsc->lvTracked)
-                {
-                    rpRecordRegIntf(RBM_CALLEE_TRASH, VarSetOps::MakeSingleton(this, pinvokeVarDsc->lvVarIndex)
-                                    DEBUGARG("compLvFrameListRoot"));
+            if (pinvokeVarDsc->lvTracked)
+            {
+                rpRecordRegIntf(RBM_CALLEE_TRASH, VarSetOps::MakeSingleton(this, pinvokeVarDsc->lvVarIndex)
+                                DEBUGARG("compLvFrameListRoot"));
 
-                    // We would prefer to have this be enregister in the PINVOKE_TCB register
-                    pinvokeVarDsc->addPrefReg(RBM_PINVOKE_TCB, this);
-                }
+                // We would prefer to have this be enregister in the PINVOKE_TCB register
+                pinvokeVarDsc->addPrefReg(RBM_PINVOKE_TCB, this);
             }
 
             //If we're using a single return block, the p/invoke epilog code trashes ESI and EDI (in the