review feedback

dotnet · Jan 11, 2021 · b100226 · b100226
1 parent 74620ed
commit b100226
Show file tree

Hide file tree

Showing 8 changed files with 46 additions and 25 deletions.
diff --git a/src/coreclr/jit/codegencommon.cpp b/src/coreclr/jit/codegencommon.cpp
@@ -2258,7 +2258,7 @@ void CodeGen::genGenerateMachineCode()
 
     GetEmitter()->emitJumpDistBind();
 
-#ifdef FEATURE_LOOP_ALIGN
+#if FEATURE_LOOP_ALIGN
     /* Perform alignment adjustments */
 
     GetEmitter()->emitLoopAlignAdjustments();

diff --git a/src/coreclr/jit/codegenlinear.cpp b/src/coreclr/jit/codegenlinear.cpp
@@ -349,11 +349,13 @@ void CodeGen::genCodeForBBlist()
             needLabel = true;
         }
 
-        if (GetEmitter()->emitCurIG->isLoopAlign())
+#if FEATURE_LOOP_ALIGN
+        if (GetEmitter()->emitEndsWithAlignInstr())
         {
             // we had better be planning on starting a new IG
             assert(needLabel);
         }
+#endif
 
         if (needLabel)
         {
@@ -745,7 +747,7 @@ void CodeGen::genCodeForBBlist()
 
             case BBJ_COND:
 
-#ifdef FEATURE_LOOP_ALIGN
+#if FEATURE_LOOP_ALIGN
                 // This is the last place where we operate on blocks and after this, we operate
                 // on IG. Hence, if we know that the destination of "block" is the first block
                 // of a loop and needs alignment (it has BBF_LOOP_ALIGN), then "block" represents
@@ -766,7 +768,7 @@ void CodeGen::genCodeForBBlist()
                 break;
         }
 
-#ifdef FEATURE_LOOP_ALIGN
+#if FEATURE_LOOP_ALIGN
 
         // If next block is the first block of a loop (identified by BBF_LOOP_ALIGN),
         // then need to add align instruction in current "block". Also mark the

diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp
@@ -641,7 +641,7 @@ void emitter::emitGenIG(insGroup* ig)
 
     assert(emitCurIGjmpList == nullptr);
 
-#ifdef FEATURE_LOOP_ALIGN
+#if FEATURE_LOOP_ALIGN
     assert(emitCurIGAlignList == nullptr);
 #endif
 
@@ -831,7 +831,7 @@ insGroup* emitter::emitSavIG(bool emitAdd)
     }
 #endif
 
-#ifdef FEATURE_LOOP_ALIGN
+#if FEATURE_LOOP_ALIGN
     // Did we have any align instructions in this group?
     if (emitCurIGAlignList)
     {
@@ -996,7 +996,7 @@ void emitter::emitBegFN(bool hasFramePtr
     emitCurIGfreeBase = nullptr;
     emitIGbuffSize    = 0;
 
-#ifdef FEATURE_LOOP_ALIGN
+#if FEATURE_LOOP_ALIGN
     emitLastAlignedIgNum        = 0;
     emitLastInnerLoopStartIgNum = 0;
     emitLastInnerLoopEndIgNum   = 0;
@@ -1037,7 +1037,7 @@ void emitter::emitBegFN(bool hasFramePtr
     emitNoGCIG     = false;
     emitForceNewIG = false;
 
-#ifdef FEATURE_LOOP_ALIGN
+#if FEATURE_LOOP_ALIGN
     /* We don't have any align instructions */
 
     emitAlignList = emitAlignLast = nullptr;
@@ -3735,7 +3735,7 @@ size_t emitter::emitIssue1Instr(insGroup* ig, instrDesc* id, BYTE** dp)
         // It is fatal to under-estimate the instruction size, except for alignment instructions
         noway_assert(estimatedSize >= actualSize);
 
-#ifdef FEATURE_LOOP_ALIGN
+#if FEATURE_LOOP_ALIGN
         // Should never over-estimate align instruction or any instruction before the last align instruction of a method
         assert(id->idIns() != INS_align && emitCurIG->igNum > emitLastAlignedIgNum);
 #endif
@@ -4610,7 +4610,7 @@ void emitter::emitJumpDistBind()
 #endif // DEBUG
 }
 
-#ifdef FEATURE_LOOP_ALIGN
+#if FEATURE_LOOP_ALIGN
 
 //-----------------------------------------------------------------------------
 // emitLoopAlignment: Insert an align instruction at the end of emitCurIG and
@@ -4636,6 +4636,16 @@ void emitter::emitLoopAlignment()
             emitComp->compMethodID, emitCurIG->igNum);
 }
 
+//-----------------------------------------------------------------------------
+//  emitEndsWithAlignInstr: Checks if current IG ends with loop align instruction.
+//
+//  Returns:  true if current IG ends with align instruciton.
+//
+bool emitter::emitEndsWithAlignInstr()
+{
+    return emitCurIG->isLoopAlign();
+}
+
 //-----------------------------------------------------------------------------
 //  getLoopSize: Starting from loopHeaderIg, find the size of the smallest possible loop
 //               such that it doesn't exceed the maxLoopSize.
@@ -7762,10 +7772,13 @@ void emitter::emitInitIG(insGroup* ig)
        sure we act the same in non-DEBUG builds.
     */
 
-    ig->igSize         = 0;
-    ig->igGCregs       = RBM_NONE;
-    ig->igInsCnt       = 0;
+    ig->igSize   = 0;
+    ig->igGCregs = RBM_NONE;
+    ig->igInsCnt = 0;
+
+#if FEATURE_LOOP_ALIGN
     ig->igLoopBackEdge = nullptr;
+#endif
 }
 
 /*****************************************************************************

diff --git a/src/coreclr/jit/emit.h b/src/coreclr/jit/emit.h
@@ -250,7 +250,10 @@ struct insGroup
     unsigned int   igFuncIdx; // Which function/funclet does this belong to? (Index into Compiler::compFuncInfos array.)
     unsigned short igFlags;   // see IGF_xxx below
     unsigned short igSize;    // # of bytes of code in this group
-    insGroup*      igLoopBackEdge; // "last" back-edge that branches back to an aligned loop head.
+
+#if FEATURE_LOOP_ALIGN
+    insGroup* igLoopBackEdge; // "last" back-edge that branches back to an aligned loop head.
+#endif
 
 #define IGF_GC_VARS 0x0001    // new set of live GC ref variables
 #define IGF_BYREF_REGS 0x0002 // new set of live by-ref registers
@@ -1370,7 +1373,7 @@ class emitter
                                   // hot to cold and cold to hot jumps)
     };
 
-#ifdef FEATURE_LOOP_ALIGN
+#if FEATURE_LOOP_ALIGN
     struct instrDescAlign : instrDesc
     {
         instrDescAlign* idaNext; // next align in the group/method
@@ -1755,7 +1758,7 @@ class emitter
     instrDescJmp* emitJumpLast;       // last of local jumps in method
     void          emitJumpDistBind(); // Bind all the local jumps in method
 
-#ifdef FEATURE_LOOP_ALIGN
+#if FEATURE_LOOP_ALIGN
     instrDescAlign* emitCurIGAlignList;                                 // list of align instructions in current IG
     unsigned        emitLastInnerLoopStartIgNum;                        // Start IG of last inner loop
     unsigned        emitLastInnerLoopEndIgNum;                          // End IG of last inner loop
@@ -1764,6 +1767,7 @@ class emitter
     instrDescAlign* emitAlignLast;                                      // last align instruction in method
     unsigned getLoopSize(insGroup* igLoopHeader, unsigned maxLoopSize); // Get the smallest loop size
     void emitLoopAlignment();
+    bool emitEndsWithAlignInstr(); // Validate if newLabel is appropriate
     void emitSetLoopBackEdge(BasicBlock* loopTopBlock);
     void     emitLoopAlignAdjustments(); // Predict if loop alignment is needed and make appropriate adjustments
     unsigned emitCalculatePaddingForLoopAlignment(insGroup* ig, size_t offset DEBUG_ARG(bool displayAlignmentDetails));
@@ -2009,7 +2013,7 @@ class emitter
         return (instrDescCGCA*)emitAllocAnyInstr(sizeof(instrDescCGCA), attr);
     }
 
-#ifdef FEATURE_LOOP_ALIGN
+#if FEATURE_LOOP_ALIGN
     instrDescAlign* emitAllocInstrAlign()
     {
 #if EMITTER_STATS
@@ -2544,7 +2548,7 @@ inline emitter::instrDescJmp* emitter::emitNewInstrJmp()
     return emitAllocInstrJmp();
 }
 
-#ifdef FEATURE_LOOP_ALIGN
+#if FEATURE_LOOP_ALIGN
 inline emitter::instrDescAlign* emitter::emitNewInstrAlign()
 {
     instrDescAlign* newInstr = emitAllocInstrAlign();

diff --git a/src/coreclr/jit/emitxarch.cpp b/src/coreclr/jit/emitxarch.cpp
@@ -7388,7 +7388,7 @@ size_t emitter::emitSizeOfInsDsc(instrDesc* id)
     switch (idOp)
     {
         case ID_OP_NONE:
-#ifdef FEATURE_LOOP_ALIGN
+#if FEATURE_LOOP_ALIGN
             if (id->idIns() == INS_align)
             {
                 return sizeof(instrDescAlign);
@@ -13814,8 +13814,9 @@ size_t emitter::emitOutputInstr(insGroup* ig, instrDesc* id, BYTE** dp)
     {
         emitDispIns(id, false, dspOffs, true, emitCurCodeOffs(*dp), *dp, (dst - *dp));
     }
+#endif
 
-#ifdef FEATURE_LOOP_ALIGN
+#if FEATURE_LOOP_ALIGN
     // Only compensate over-estimated instructions if emitCurIG is before
     // the last IG that needs alignment.
     if (emitCurIG->igNum <= emitLastAlignedIgNum)
@@ -13852,6 +13853,7 @@ size_t emitter::emitOutputInstr(insGroup* ig, instrDesc* id, BYTE** dp)
     }
 #endif
 
+#ifdef DEBUG
     if (emitComp->compDebugBreak)
     {
         // set JitEmitPrintRefRegs=1 will print out emitThisGCrefRegs and emitThisByrefRegs

diff --git a/src/coreclr/jit/jitconfigvalues.h b/src/coreclr/jit/jitconfigvalues.h
@@ -223,7 +223,7 @@ CONFIG_INTEGER(EnableIncompleteISAClass, W("EnableIncompleteISAClass"), 0) // En
                                                                            // intrinsic classes
 #endif                                                                     // defined(DEBUG)
 
-#ifdef FEATURE_LOOP_ALIGN
+#if FEATURE_LOOP_ALIGN
 CONFIG_INTEGER(JitAlignLoops, W("JitAlignLoops"), 1) // If set, align inner loops
 #else
 CONFIG_INTEGER(JitAlignLoops, W("JitAlignLoops"), 0)

diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp
@@ -16317,7 +16317,7 @@ bool Compiler::fgFoldConditional(BasicBlock* block)
                          * Remove the loop from the table */
 
                         optLoopTable[loopNum].lpFlags |= LPFLG_REMOVED;
-#ifdef FEATURE_LOOP_ALIGN
+#if FEATURE_LOOP_ALIGN
                         optLoopTable[loopNum].lpFirst->bbFlags &= ~BBF_LOOP_ALIGN;
                         JITDUMP("Removing LOOP_ALIGN flag from bogus loop in " FMT_BB "\n",
                                 optLoopTable[loopNum].lpFirst->bbNum);

diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp
@@ -2586,7 +2586,7 @@ void Compiler::optFindNaturalLoops()
 
 void Compiler::optIdentifyLoopsForAlignment()
 {
-#ifdef FEATURE_LOOP_ALIGN
+#if FEATURE_LOOP_ALIGN
     if (codeGen->ShouldAlignLoops())
     {
         for (unsigned char loopInd = 0; loopInd < optLoopCount; loopInd++)
@@ -3792,7 +3792,7 @@ void Compiler::optUnrollLoops()
 #endif
         }
 
-#ifdef FEATURE_LOOP_ALIGN
+#if FEATURE_LOOP_ALIGN
         for (block = head->bbNext;; block = block->bbNext)
         {
             if (block->isLoopAlign())
@@ -8032,7 +8032,7 @@ bool Compiler::optComputeLoopSideEffectsOfBlock(BasicBlock* blk)
 void Compiler::AddContainsCallAllContainingLoops(unsigned lnum)
 {
 
-#ifdef FEATURE_LOOP_ALIGN
+#if FEATURE_LOOP_ALIGN
     // If this is the inner most loop, reset the LOOP_ALIGN flag
     // because a loop having call will not likely to benefit from
     // alignment