Loop alignment: Fix loop size calculation to exclude IG's size that are marked as not aligned

src/coreclr/jit/compiler.cpp

@@ -5154,6 +5154,8 @@ void Compiler::placeLoopAlignInstructions()
  {
  // Adding align instruction in prolog is not supported
  // hence just remove that loop from our list.
+ fgFirstBB->bbFlags &= ~BBF_LOOP_ALIGN;
+ JITDUMP("Removing LOOP_ALIGN flag from prolog " FMT_BB "\n", fgFirstBB->bbNum);
  loopsToProcess--;
  }
 

src/coreclr/jit/emit.cpp

@@ -1040,6 +1040,7 @@ void emitter::emitBegFN(bool hasFramePtr
  emitLastAlignedIgNum = 0;
  emitLastLoopStart = 0;
  emitLastLoopEnd = 0;
+ emitLoopAlignRemoved = false;
 #endif
 
  /* Record stack frame info (the temp size is just an estimate) */
@@ -5078,6 +5079,52 @@ bool emitter::emitEndsWithAlignInstr()
  return emitCurIG->endsWithAlignInstr();
 }
 
+//-----------------------------------------------------------------------------
+// emitRemoveIGAlignFlag: Remove the align flag of `alignInstr`.
+//
+void emitter::emitRemoveIGAlignFlag(instrDescAlign* alignInstr)
+{
+ emitLoopAlignRemoved = true;
+ alignInstr->removeAlignFlags();
+}
+
+//-----------------------------------------------------------------------------
+// emitIGHadAlignFlag: Checks if 'igToCheck' ever was marked as aligned and later
+// decided to not align. Sometimes, a loop is marked as not
+// needing alignment, but the igSize was not adjusted immediately.
+// This method would helpful to know that specially during loopSize
+// calculation where we would adjust the loopsize by removed alignment
+// bytes.
+//
+// Returns: true iff igToCheck had alignment but was later removed. For any other IG
+// whether was marked aligned or not, the method will return false.
+//
+bool emitter::emitIGHadAlignFlag(insGroup* igToCheck)
+{
+ if (!emitLoopAlignRemoved || igToCheck->endsWithAlignInstr())
+ {
+ // If we never removed align flag of any loop or if
+ // the IG already has an align flag, it means it was not removed.
+ return false;
+ }
+
+ // Otherwise scan the align instruction list and see if the
+ // `igToCheck` was present.
+ instrDescAlign* alignInstr = emitAlignList;
+ while ((alignInstr != nullptr))
+ {
+ if (alignInstr->idaIG == igToCheck)
+ {
+ return true;
+ }
+ alignInstr = emitAlignInNextIG(alignInstr);
+ }
+
+ // `igToCheck` was not part of the align instruction and
+ // hence the 'align' flag was never removed from it.
+ return false;
+}
+
 //-----------------------------------------------------------------------------
 // getLoopSize: Starting from loopHeaderIg, find the size of the smallest possible loop
 // such that it doesn't exceed the maxLoopSize.
@@ -5096,10 +5143,14 @@ unsigned emitter::getLoopSize(insGroup* igLoopHeader, unsigned maxLoopSize DEBUG
 {
  unsigned loopSize = 0;
 
+ JITDUMP("*************** In getLoopSize() for %s\n", emitLabelString(igLoopHeader), loopSize);
+
  for (insGroup* igInLoop = igLoopHeader; igInLoop != nullptr; igInLoop = igInLoop->igNext)
  {
  loopSize += igInLoop->igSize;
- if (igInLoop->endsWithAlignInstr())
+ JITDUMP(" %s has %u bytes.", emitLabelString(igInLoop), igInLoop->igSize);
+
+ if (igInLoop->endsWithAlignInstr() || emitIGHadAlignFlag(igInLoop))
  {
  // If IGF_HAS_ALIGN is present, igInLoop contains align instruction at the end,
  // for next IG or some future IG.
@@ -5179,16 +5230,31 @@ unsigned emitter::getLoopSize(insGroup* igLoopHeader, unsigned maxLoopSize DEBUG
  else
 #endif
  {
+ JITDUMP(" but ends with align instruction, taking off %u bytes.",
+ emitComp->opts.compJitAlignPaddingLimit);
  // The current loop size should exclude the align instruction size reserved for next loop.
  loopSize -= emitComp->opts.compJitAlignPaddingLimit;
  }
  }
  if ((igInLoop->igLoopBackEdge == igLoopHeader) || (loopSize > maxLoopSize))
  {
+#ifdef DEBUG
+ if (igInLoop->igLoopBackEdge == igLoopHeader)
+ {
+ JITDUMP(" -- Found the back edge.");
+ }
+ else
+ {
+ JITDUMP(" -- loopSize exceeded the threshold of %u bytes.", maxLoopSize);
+ }
+ JITDUMP("\n");
  break;
+#endif
  }
+ JITDUMP("\n");
  }
 
+ JITDUMP("loopSize of %s = %u bytes.\n", emitLabelString(igLoopHeader), loopSize);
  return loopSize;
 }
 
@@ -5279,10 +5345,10 @@ void emitter::emitSetLoopBackEdge(BasicBlock* loopTopBlock)
  assert(!markedCurrLoop);
 
  // This IG should no longer contain alignment instruction
- alignInstr->removeAlignFlags();
+ emitRemoveIGAlignFlag(alignInstr);
 
  markedCurrLoop = true;
- JITDUMP("** Skip alignment for current loop IG%02u ~ IG%02u because it encloses an aligned loop "
+ JITDUMP(";; Skip alignment for current loop IG%02u ~ IG%02u because it encloses an aligned loop "
  "IG%02u ~ IG%02u.\n",
  currLoopStart, currLoopEnd, emitLastLoopStart, emitLastLoopEnd);
  }
@@ -5292,13 +5358,13 @@ void emitter::emitSetLoopBackEdge(BasicBlock* loopTopBlock)
  if (!alignLastLoop && (loopHeadIG != nullptr) && (loopHeadIG->igNum == emitLastLoopStart))
  {
  assert(!markedLastLoop);
- assert(alignInstr->idaIG->endsWithAlignInstr());
+ assert(alignInstr->idaIG->endsWithAlignInstr() || emitIGHadAlignFlag(alignInstr->idaIG));
 
  // This IG should no longer contain alignment instruction
- alignInstr->removeAlignFlags();
+ emitRemoveIGAlignFlag(alignInstr);
 
  markedLastLoop = true;
- JITDUMP("** Skip alignment for aligned loop IG%02u ~ IG%02u because it encloses the current loop "
+ JITDUMP(";; Skip alignment for aligned loop IG%02u ~ IG%02u because it encloses the current loop "
  "IG%02u ~ IG%02u.\n",
  emitLastLoopStart, emitLastLoopEnd, currLoopStart, currLoopEnd);
  }
@@ -5388,7 +5454,7 @@ void emitter::emitLoopAlignAdjustments()
  containingIG->igFlags |= IGF_UPD_ISZ;
  if (actualPaddingNeeded == 0)
  {
- alignInstr->removeAlignFlags();
+ emitRemoveIGAlignFlag(alignInstr);
  }
 
 #ifdef TARGET_XARCH

src/coreclr/jit/emit.h

@@ -1992,6 +1992,7 @@ class emitter
  unsigned emitLastAlignedIgNum; // last IG that has align instruction
  instrDescAlign* emitAlignList; // list of all align instructions in method
  instrDescAlign* emitAlignLast; // last align instruction in method
+ bool emitLoopAlignRemoved; // quick way to check if any loop's alignment was ever removed.
 
  // Points to the most recent added align instruction. If there are multiple align instructions like in arm64 or
  // non-adaptive alignment on xarch, this points to the first align instruction of the series of align instructions.
@@ -2001,6 +2002,8 @@ class emitter
  unsigned maxLoopSize DEBUG_ARG(bool isAlignAdjusted)); // Get the smallest loop size
  void emitLoopAlignment(DEBUG_ARG1(bool isPlacedBehindJmp));
  bool emitEndsWithAlignInstr(); // Validate if newLabel is appropriate
+ void emitRemoveIGAlignFlag(instrDescAlign* alignInstr);
+ bool emitIGHadAlignFlag(insGroup* igToCheck);
  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 isAlignAdjusted));

src/coreclr/jit/optimizer.cpp

@@ -2625,7 +2625,7 @@ void Compiler::optIdentifyLoopsForAlignment()
  }
  else
  {
- JITDUMP("Skip alignment for " FMT_LP " that starts at " FMT_BB " weight=" FMT_WT ".\n", loopInd,
+ JITDUMP(";; Skip alignment for " FMT_LP " that starts at " FMT_BB " weight=" FMT_WT ".\n", loopInd,
  top->bbNum, topWeight);
  }
  }

src/tests/JIT/Regression/JitBlue/GitHub_65690/GitHub_65690.cs

@@ -0,0 +1,120 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+//
+// Note: In below test case, there are two backedges to a loop that is marked for align.
+// If we see intersecting aligned loops, we mark that loop as not needing alignment
+// but then when we see 2nd backedge we again try to mark it as not needing alignment
+// and that triggers an assert.
+// Found by Antigen
+using System;
+using System.Collections.Generic;
+using System.Runtime.CompilerServices;
+public class TestClass_65690
+{
+ public struct S1
+ {
+ public struct S1_D1_F1
+ {
+ public decimal decimal_0;
+ public bool bool_1;
+ public sbyte sbyte_2;
+ }
+ public double double_3;
+ }
+ public struct S2
+ {
+ public S1.S1_D1_F1 s1_s1_d1_f1_5;
+ }
+ static byte s_byte_7 = 2;
+ static decimal s_decimal_9 = -1.9647058823529411764705882353m;
+ static int s_int_12 = 1;
+ static long s_long_13 = -2147483647;
+ static ulong s_ulong_19 = 1;
+ static S1.S1_D1_F1 s_s1_s1_d1_f1_20 = new S1.S1_D1_F1();
+ bool bool_23 = true;
+ byte byte_24 = 5;
+ int int_29 = 1;
+ ushort ushort_34 = 5;
+ uint uint_35 = 5;
+ S1.S1_D1_F1 s1_s1_d1_f1_37 = new S1.S1_D1_F1();
+ S1 s1_38 = new S1();
+ S2 s2_39 = new S2();
+ static int s_loopInvariant = 4;
+ [MethodImpl(MethodImplOptions.NoInlining)]
+ public byte LeafMethod1()
+ {
+ unchecked
+ {
+ return 15 + 4;
+ }
+ }
+
+ public double Method3(out sbyte p_sbyte_93, out double p_double_94, out S1.S1_D1_F1 p_s1_s1_d1_f1_98)
+ {
+ unchecked
+ {
+ p_double_94 = 15 + 4;
+ p_s1_s1_d1_f1_98 = new S1.S1_D1_F1();
+ p_sbyte_93 = 15 % 4;
+ return 15 + 4;
+ }
+ }
+
+ public void Method0()
+ {
+ unchecked
+ {
+ int int_145 = -5;
+ ulong ulong_152 = 5;
+ S1.S1_D1_F1 s1_s1_d1_f1_153 = new S1.S1_D1_F1();
+ S2 s2_155 = new S2();
+ if (15 - 4 != LeafMethod1())
+ {
+ }
+ else
+ {
+ try
+ {
+ ulong_152 >>= s_int_12 = int_145 -= 15 + 4;
+ }
+ finally
+ {
+ s2_39.s1_s1_d1_f1_5.decimal_0 = 15 % 4 + s1_s1_d1_f1_153.decimal_0 - 15 + 4 + 40;
+ }
+ int __loopvar25 = 15 + 4;
+ do
+ {
+ if (__loopvar25 < s_loopInvariant - 1)
+ break;
+ }
+ while (s1_s1_d1_f1_37.bool_1 = s1_s1_d1_f1_37.bool_1 = bool_23);
+ }
+ if (15 / 4 % ulong_152 + 22 + 78 - s_ulong_19 > 19)
+ {
+ int __loopvar26 = 15 - 4;
+ for (; ; )
+ {
+ if (__loopvar26 >= s_loopInvariant)
+ break;
+ if (s1_s1_d1_f1_37.bool_1)
+ {
+ Method3(out s2_155.s1_s1_d1_f1_5.sbyte_2, out s1_38.double_3, out s1_s1_d1_f1_153);
+ }
+ else
+ {
+ ushort_34 *= 15 % 4;
+ }
+ if ((byte_24 = s_byte_7 ^= 15 | 4) >= (15 | 4))
+ {
+ }
+ }
+ }
+ return;
+ }
+ }
+ public static int Main(string[] args)
+ {
+ new TestClass_65690().Method0();
+ return 100;
+ }
+}

src/tests/JIT/Regression/JitBlue/GitHub_65690/GitHub_65690.csproj

@@ -0,0 +1,27 @@
+<Project Sdk="Microsoft.NET.Sdk">
+ <PropertyGroup>
+ <OutputType>Exe</OutputType>
+ <AllowUnsafeBlocks>True</AllowUnsafeBlocks>
+ </PropertyGroup>
+ <PropertyGroup>
+ <DebugType>None</DebugType>
+ <Optimize>True</Optimize>
+ </PropertyGroup>
+ <PropertyGroup>
+ <CLRTestBatchPreCommands><![CDATA[
+$(CLRTestBatchPreCommands)
+set COMPlus_JitDoValueNumber=0
+set COMPlus_JitStressRegs=3
+set COMPlus_TieredCompilation=0
+]]></CLRTestBatchPreCommands>
+ <BashCLRTestPreCommands><![CDATA[
+$(BashCLRTestPreCommands)
+export COMPlus_JitDoValueNumber=0
+export COMPlus_JitStressRegs=3
+export COMPlus_TieredCompilation=0
+]]></BashCLRTestPreCommands>
+ </PropertyGroup>
+ <ItemGroup>
+ <Compile Include="$(MSBuildProjectName).cs" />
+ </ItemGroup>
+</Project>

src/tests/JIT/Regression/JitBlue/GitHub_65988/GitHub_65988.cs

@@ -0,0 +1,48 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+//
+// Note: In below test case, we were skipping the first block that is an alignment candidate,
+// but were not unmarking it such. As a result, we would hit assert during backedge setup.
+// Found by Antigen
+public class TestClass_65988
+{
+ public struct S1
+ {
+ public decimal decimal_0;
+ }
+ public struct S2
+ {
+ public S1 s1_2;
+ }
+ static int s_int_9 = 1;
+ decimal decimal_22 = 0.08m;
+ S1 s1_33 = new S1();
+ public decimal LeafMethod3() => 67.1m;
+
+ public void Method0()
+ {
+ unchecked
+ {
+ int int_85 = 76;
+ S1 s1_93 = new S1();
+ for (; ; )
+ {
+ if ((15 << 4) < (s_int_9 *= int_85))
+ {
+ s1_33.decimal_0 += decimal_22 -= 15 * 4 - -2147483646.9375m;
+ }
+ s1_93.decimal_0 /= (s1_33.decimal_0 /= LeafMethod3()) + 28;
+ if (int_85++ == 77000)
+ {
+ break;
+ }
+ }
+ return;
+ }
+ }
+ public static int Main(string[] args)
+ {
+ new TestClass_65988().Method0();
+ return 100;
+ }
+}