Address PR feedback.

8 files changed, 140 insertions, 12 deletions

diff --git a/src/jit/codegenarm64.cpp b/src/jit/codegenarm64.cpp
index ca0df53a34..14d20521a1 100644
--- a/src/jit/codegenarm64.cpp
+++ b/src/jit/codegenarm64.cpp
@@ -5147,7 +5147,7 @@ void CodeGen::genConsumeRegs(GenTree* tree)
         }
         else if (tree->OperGet() == GT_AND)
         {
-            // This is the special contained GT_AND that we created in Lowering::LowerCmp()
+            // This is the special contained GT_AND that we created in Lowering::TreeNodeInfoInitCmp()
             // Now we need to consume the operands of the GT_AND node.
             genConsumeOperands(tree->AsOp());
         }
diff --git a/src/jit/codegenxarch.cpp b/src/jit/codegenxarch.cpp
index f7159997b3..0538a665b2 100644
--- a/src/jit/codegenxarch.cpp
+++ b/src/jit/codegenxarch.cpp
@@ -2461,6 +2461,13 @@ void CodeGen::genCodeForTreeNode(GenTreePtr treeNode)
             else if (varTypeIsLong(op1Type))
             {
 #ifdef DEBUG
+                // The result of an unlowered long compare on a 32-bit target must either be
+                // a) materialized into a register, or
+                // b) unused.
+                //
+                // A long compare that has a result that is used but not materialized into a register should
+                // have been handled by Lowering::LowerCompare.
+
                 LIR::Use use;
                 assert((treeNode->gtRegNum != REG_NA) || !LIR::AsRange(compiler->compCurBB).TryGetUse(treeNode, &use));
 #endif
@@ -5230,7 +5237,7 @@ void CodeGen::genConsumeRegs(GenTree* tree)
         }
         else if (tree->OperGet() == GT_AND)
         {
-            // This is the special contained GT_AND that we created in Lowering::LowerCmp()
+            // This is the special contained GT_AND that we created in Lowering::TreeNodeInfoInitCmp()
             // Now we need to consume the operands of the GT_AND node.
             genConsumeOperands(tree->AsOp());
         }
@@ -7303,7 +7310,7 @@ void CodeGen::genCompareInt(GenTreePtr treeNode)
     {
         // Do we have a short compare against a constant in op2?
         //
-        // We checked for this case in LowerCmp() and if we can perform a small
+        // We checked for this case in TreeNodeInfoInitCmp() and if we can perform a small
         // compare immediate we labeled this compare with a GTF_RELOP_SMALL
         // and for unsigned small non-equality compares the GTF_UNSIGNED flag.
         //
@@ -7358,7 +7365,7 @@ void CodeGen::genCompareInt(GenTreePtr treeNode)
         if (op1->isContained())
         {
             // op1 can be a contained memory op
-            // or the special contained GT_AND that we created in Lowering::LowerCmp()
+            // or the special contained GT_AND that we created in Lowering::TreeNodeInfoInitCmp()
             //
             if ((op1->OperGet() == GT_AND))
             {
diff --git a/src/jit/flowgraph.cpp b/src/jit/flowgraph.cpp
index 9e6fd1cda7..97eb5cdaee 100644
--- a/src/jit/flowgraph.cpp
+++ b/src/jit/flowgraph.cpp
@@ -19945,7 +19945,7 @@ void                Compiler::fgDebugCheckBBlist(bool checkBBNum  /* = false */,
 
         // If the block is a BBJ_COND, a BBJ_SWITCH or a
         // lowered GT_SWITCH_TABLE node then make sure it
-        // ends with a jump or a GT_SWITCH
+        // ends with a conditional jump or a GT_SWITCH
 
         if (block->bbJumpKind == BBJ_COND)
         {
diff --git a/src/jit/gtlist.h b/src/jit/gtlist.h
index 15ab9333b3..da8246d20e 100644
--- a/src/jit/gtlist.h
+++ b/src/jit/gtlist.h
@@ -189,6 +189,9 @@ GTNODE(SIMD       , "simd"       ,0,GTK_BINOP|GTK_EXOP)   // SIMD functions/oper
 //  Other nodes that look like unary/binary operators:
 //-----------------------------------------------------------------------------
 
+// The following are both conditional branches. GT_JTRUE has a single operand that computes a condition. GT_JCC
+// implicitly reads the condition bits from a previous operation. The latter is allowed only in the LIR form
+// used in the RyuJIT backend.
 GTNODE(JTRUE      , "jmpTrue"    ,0,GTK_UNOP|GTK_NOVALUE)
 GTNODE(JCC        , "jcc"        ,0,GTK_LEAF|GTK_NOVALUE)
 
diff --git a/src/jit/lower.cpp b/src/jit/lower.cpp
index e0d840f449..6a90c454c6 100644
--- a/src/jit/lower.cpp
+++ b/src/jit/lower.cpp
@@ -1872,6 +1872,72 @@ GenTree* Lowering::LowerTailCallViaHelper(GenTreeCall* call, GenTree* callTarget
     return result;
 }
 
+//------------------------------------------------------------------------
+// Lowering::LowerCompare: lowers a compare node.
+//
+// For 64-bit targets, this doesn't do much of anything: all comparisons
+// that we support can be handled in code generation on such targets.
+//
+// For 32-bit targets, however, any comparison that feeds a `GT_JTRUE`
+// node must be lowered such that the liveness of the operands to the
+// comparison is properly visible to the rest of the backend. As such,
+// a 64-bit comparison is lowered from something like this:
+//
+//    ------------ BB02 [004..014) -> BB02 (cond), preds={BB02,BB01} succs={BB03,BB02}
+//    N001 (  1,  1) [000006] ------------        t6 =    lclVar    int    V02 loc0         u:5 $148
+//
+//                                                     /--*  t6     int
+//    N002 (  2,  3) [000007] ---------U--        t7 = *  cast      long <- ulong <- uint $3c0
+//
+//    N003 (  3, 10) [000009] ------------        t9 =    lconst    long   0x0000000000000003 $101
+//
+//                                                     /--*  t7     long
+//                                                     +--*  t9     long
+//    N004 (  9, 17) [000010] N------N-U--       t10 = *  <         int    $149
+//
+//                                                     /--*  t10    int
+//    N005 ( 11, 19) [000011] ------------             *  jmpTrue   void
+//
+// To something like this:
+//
+//    ------------ BB02 [004..014) -> BB03 (cond), preds={BB06,BB07,BB01} succs={BB06,BB03}
+//                   [000099] ------------       t99 =    const     int    0
+//
+//                   [000101] ------------      t101 =    const     int    0
+//
+//                                                     /--*  t99    int
+//                                                     +--*  t101   int
+//    N004 (  9, 17) [000010] N------N-U--       t10 = *  >         int    $149
+//
+//                                                     /--*  t10    int
+//    N005 ( 11, 19) [000011] ------------             *  jmpTrue   void
+//
+//
+//    ------------ BB06 [???..???) -> BB02 (cond), preds={BB02} succs={BB07,BB02}
+//                   [000105] -------N-U--                jcc       void   cond=<
+//
+//
+//    ------------ BB07 [???..???) -> BB02 (cond), preds={BB06} succs={BB03,BB02}
+//    N001 (  1,  1) [000006] ------------        t6 =    lclVar    int    V02 loc0         u:5 $148
+//
+//    N003 (  3, 10) [000009] ------------        t9 =    const     int    3
+//
+//                                                     /--*  t6     int
+//                                                     +--*  t9     int
+//                   [000106] N------N-U--      t106 = *  <         int
+//
+//                                                     /--*  t106   int
+//                   [000107] ------------             *  jmpTrue   void
+//
+// Which will eventually generate code similar to the following:
+//
+//    33DB         xor      ebx, ebx
+//    85DB         test     ebx, ebx
+//    7707         ja       SHORT G_M50523_IG04
+//    72E7         jb       SHORT G_M50523_IG03
+//    83F803       cmp      eax, 3
+//    72E2         jb       SHORT G_M50523_IG03
+//
 void Lowering::LowerCompare(GenTree* cmp)
 {
 #ifndef _TARGET_64BIT_
@@ -1914,6 +1980,28 @@ void Lowering::LowerCompare(GenTree* cmp)
 
     if (cmp->OperGet() == GT_EQ || cmp->OperGet() == GT_NE)
     {
+        // 64-bit equality comparisons (no matter the polarity) require two 32-bit comparisons: one for the upper 32
+        // bits and one for the lower 32 bits. As such, we update the flow graph like so:
+        //
+        //     Before:
+        //                 BB0: cond
+        //                   /   \
+        //                false  true
+        //                  |     |
+        //                 BB1   BB2
+        //
+        //     After:
+        //                  BB0: cond(hi)
+        //                   /        \
+        //                false       true
+        //                  |          |
+        //                  |     BB3: cond(lo)
+        //                  |      /       \
+        //                  |   false      true
+        //                  \    /          |
+        //                    BB1          BB2
+        //
+
         BlockRange().Remove(loSrc1.Def());
         BlockRange().Remove(loSrc2.Def());
         GenTree* loCmp = comp->gtNewOperNode(cmp->OperGet(), TYP_INT, loSrc1.Def(), loSrc2.Def());
@@ -1941,6 +2029,37 @@ void Lowering::LowerCompare(GenTree* cmp)
     }
     else
     {
+        // 64-bit ordinal comparisons are more complicated: they require two comparisons for the upper 32 bits and one
+        // comparison for the lower 32 bits. We update the flowgraph as such:
+        //
+        //     Before:
+        //                 BB0: cond
+        //                   /   \
+        //                false  true
+        //                  |     |
+        //                 BB1   BB2
+        //
+        //     After:
+        //           BB0: (!cond(hi) && !eq(hi))
+        //               /                \
+        //             true              false
+        //              |                  |
+        //              |      BB3: (cond(hi) && !eq(hi))
+        //              |             /        \
+        //              |          false      true
+        //              |            |          |
+        //              |      BB4: cond(lo)    |
+        //              |       /         \     |
+        //              |    false        true  |
+        //              \     /             \   /
+        //                BB1                BB2
+        //
+        //
+        // Note that the actual comparisons used to implement "(!cond(hi) && !eq(hi))" and "(cond(hi) && !eq(hi))"
+        // differ based on the original condition, and all consist of a single node. The switch statement below
+        // performs the necessary mapping.
+        //
+
         genTreeOps hiCmpOper;
         genTreeOps loCmpOper;
         
diff --git a/src/jit/lower.h b/src/jit/lower.h
index dc6d867b09..5ce21e48f4 100644
--- a/src/jit/lower.h
+++ b/src/jit/lower.h
@@ -190,6 +190,7 @@ private:
     void TreeNodeInfoInitReturn(GenTree* tree);
     void TreeNodeInfoInitShiftRotate(GenTree* tree);
     void TreeNodeInfoInitCall(GenTreeCall* call);
+    void TreeNodeInfoInitCmp(GenTreePtr tree);
     void TreeNodeInfoInitStructArg(GenTreePtr structArg);
     void TreeNodeInfoInitBlockStore(GenTreeBlk* blkNode);
     void TreeNodeInfoInitLogicalOp(GenTree* tree);
@@ -226,8 +227,6 @@ private:
     void SetMulOpCounts(GenTreePtr tree);
 #endif // defined(_TARGET_XARCH_)
 
-    void LowerCmp(GenTreePtr tree);
-
 #if !CPU_LOAD_STORE_ARCH
     bool IsBinOpInRMWStoreInd(GenTreePtr tree);
     bool IsRMWMemOpRootedAtStoreInd(GenTreePtr storeIndTree, GenTreePtr* indirCandidate, GenTreePtr* indirOpSource);
diff --git a/src/jit/lowerarm64.cpp b/src/jit/lowerarm64.cpp
index 709639d931..1b9e40e603 100644
--- a/src/jit/lowerarm64.cpp
+++ b/src/jit/lowerarm64.cpp
@@ -484,7 +484,7 @@ void Lowering::TreeNodeInfoInit(GenTree* tree)
         case GT_LE:
         case GT_GE:
         case GT_GT:
-            LowerCmp(tree);
+            TreeNodeInfoInitCmp(tree);
             break;
 
         case GT_CKFINITE:
@@ -1859,7 +1859,7 @@ void Lowering::SetIndirAddrOpCounts(GenTreePtr indirTree)
     }
 }
 
-void Lowering::LowerCmp(GenTreePtr tree)
+void Lowering::TreeNodeInfoInitCmp(GenTreePtr tree)
 {
     TreeNodeInfo* info = &(tree->gtLsraInfo);
 
diff --git a/src/jit/lowerxarch.cpp b/src/jit/lowerxarch.cpp
index cbd85e3a51..c82603ba46 100644
--- a/src/jit/lowerxarch.cpp
+++ b/src/jit/lowerxarch.cpp
@@ -506,7 +506,7 @@ void Lowering::TreeNodeInfoInit(GenTree* tree)
         case GT_LE:
         case GT_GE:
         case GT_GT:
-            LowerCmp(tree);
+            TreeNodeInfoInitCmp(tree);
             break;
 
         case GT_CKFINITE:
@@ -2899,7 +2899,7 @@ void Lowering::SetIndirAddrOpCounts(GenTreePtr indirTree)
     }
 }
 
-void Lowering::LowerCmp(GenTreePtr tree)
+void Lowering::TreeNodeInfoInitCmp(GenTreePtr tree)
 {
     TreeNodeInfo* info = &(tree->gtLsraInfo);
 
@@ -3265,7 +3265,7 @@ void Lowering::LowerCmp(GenTreePtr tree)
 #ifdef DEBUG
                         if (comp->verbose)
                         {
-                            printf("LowerCmp: Removing a GT_CAST to TYP_UBYTE and changing castOp1->gtType to "
+                            printf("TreeNodeInfoInitCmp: Removing a GT_CAST to TYP_UBYTE and changing castOp1->gtType to "
                                    "TYP_UBYTE\n");
                             comp->gtDispTreeRange(BlockRange(), tree);
                         }