From 01cef3ea9ad6058f0e957ddc43640fa51efbf674 Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Wed, 4 May 2022 12:22:29 -0700 Subject: [PATCH 1/8] Flag to track IGs whose alignment was removed --- src/coreclr/jit/emit.cpp | 31 ++++++++++++++++++++++++++++--- src/coreclr/jit/emit.h | 2 ++ src/coreclr/jit/optimizer.cpp | 2 +- 3 files changed, 31 insertions(+), 4 deletions(-) diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index 0b8013a693a48..603a9e59aa666 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -5096,10 +5096,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() /*|| (igInLoop->igFlags & IGF_REMOVED_ALIGN)*/) { // If IGF_HAS_ALIGN is present, igInLoop contains align instruction at the end, // for next IG or some future IG. @@ -5179,16 +5183,35 @@ 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; } } + else if (igInLoop->igFlags & IGF_REMOVED_ALIGN) + { + assert("!Failed to remove 15 bytes"); + } 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; } @@ -5280,9 +5303,10 @@ void emitter::emitSetLoopBackEdge(BasicBlock* loopTopBlock) // This IG should no longer contain alignment instruction alignInstr->removeAlignFlags(); + alignInstr->idaIG->igFlags |= IGF_REMOVED_ALIGN; 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); } @@ -5296,9 +5320,10 @@ void emitter::emitSetLoopBackEdge(BasicBlock* loopTopBlock) // This IG should no longer contain alignment instruction alignInstr->removeAlignFlags(); + alignInstr->idaIG->igFlags |= IGF_REMOVED_ALIGN; 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); } diff --git a/src/coreclr/jit/emit.h b/src/coreclr/jit/emit.h index a19b912b3e17c..cb7f0dc65355a 100644 --- a/src/coreclr/jit/emit.h +++ b/src/coreclr/jit/emit.h @@ -277,6 +277,8 @@ struct insGroup // and the emitter should continue to track GC info as if there was no new block. #define IGF_HAS_ALIGN 0x0400 // this group contains an alignment instruction(s) at the end to align either the next // IG, or, if this IG contains with an unconditional branch, some subsequent IG. +#define IGF_REMOVED_ALIGN 0x0800 // this group had an alignment instruction(s) which was later removed without updating + // the IG's size/offsets. // Mask of IGF_* flags that should be propagated to new blocks when they are created. // This allows prologs and epilogs to be any number of IGs, but still be diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index 9be1521030b51..54b5af5a45a8f 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/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); } } From c9da12584f7e2960e4583c0026b9f60f9f136de0 Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Wed, 4 May 2022 13:37:26 -0700 Subject: [PATCH 2/8] Update src/coreclr/jit/emit.cpp Co-authored-by: Wraith --- src/coreclr/jit/emit.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index 603a9e59aa666..11fdf5e6f7db6 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -5191,7 +5191,7 @@ unsigned emitter::getLoopSize(insGroup* igLoopHeader, unsigned maxLoopSize DEBUG } else if (igInLoop->igFlags & IGF_REMOVED_ALIGN) { - assert("!Failed to remove 15 bytes"); + assert(!"Failed to remove 15 bytes"); } if ((igInLoop->igLoopBackEdge == igLoopHeader) || (loopSize > maxLoopSize)) { From 2322a67031eaf62f1f832e24682e92935bbca256 Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Thu, 5 May 2022 08:02:22 -0700 Subject: [PATCH 3/8] Fix for 65988: Unmark the firstBB as not aligned --- src/coreclr/jit/compiler.cpp | 2 + .../JitBlue/GitHub_65988/GitHub_65988.cs | 48 +++++++++++++++++++ .../JitBlue/GitHub_65988/GitHub_65988.csproj | 31 ++++++++++++ 3 files changed, 81 insertions(+) create mode 100644 src/tests/JIT/Regression/JitBlue/GitHub_65988/GitHub_65988.cs create mode 100644 src/tests/JIT/Regression/JitBlue/GitHub_65988/GitHub_65988.csproj diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index 900e89318ec59..c0fdafef04da3 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/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--; } diff --git a/src/tests/JIT/Regression/JitBlue/GitHub_65988/GitHub_65988.cs b/src/tests/JIT/Regression/JitBlue/GitHub_65988/GitHub_65988.cs new file mode 100644 index 0000000000000..9297416bf61af --- /dev/null +++ b/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; + } +} \ No newline at end of file diff --git a/src/tests/JIT/Regression/JitBlue/GitHub_65988/GitHub_65988.csproj b/src/tests/JIT/Regression/JitBlue/GitHub_65988/GitHub_65988.csproj new file mode 100644 index 0000000000000..31a6649adeec6 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/GitHub_65988/GitHub_65988.csproj @@ -0,0 +1,31 @@ + + + Exe + True + + + None + True + + + + + + + + + \ No newline at end of file From 1d9bdfed454a3c99ec6d3e709d85d57179dd45d9 Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Thu, 5 May 2022 13:47:17 -0700 Subject: [PATCH 4/8] Add test case for #65690 --- .../JitBlue/GitHub_65690/GitHub_65690.cs | 120 ++++++++++++++++++ .../JitBlue/GitHub_65690/GitHub_65690.csproj | 27 ++++ 2 files changed, 147 insertions(+) create mode 100644 src/tests/JIT/Regression/JitBlue/GitHub_65690/GitHub_65690.cs create mode 100644 src/tests/JIT/Regression/JitBlue/GitHub_65690/GitHub_65690.csproj diff --git a/src/tests/JIT/Regression/JitBlue/GitHub_65690/GitHub_65690.cs b/src/tests/JIT/Regression/JitBlue/GitHub_65690/GitHub_65690.cs new file mode 100644 index 0000000000000..aaed015d371e6 --- /dev/null +++ b/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; + } +} \ No newline at end of file diff --git a/src/tests/JIT/Regression/JitBlue/GitHub_65690/GitHub_65690.csproj b/src/tests/JIT/Regression/JitBlue/GitHub_65690/GitHub_65690.csproj new file mode 100644 index 0000000000000..5bca6e2d62cfb --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/GitHub_65690/GitHub_65690.csproj @@ -0,0 +1,27 @@ + + + Exe + True + + + None + True + + + + + + + + + \ No newline at end of file From 3704be5c435edfadaeba69b9d3c5f302e8f2e118 Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Thu, 5 May 2022 13:55:15 -0700 Subject: [PATCH 5/8] Replace IGF_REMOVE_FLAG with different logic --- src/coreclr/jit/emit.cpp | 63 +++++++++++++++++++++++++++++++++------- src/coreclr/jit/emit.h | 5 ++-- 2 files changed, 55 insertions(+), 13 deletions(-) diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index 11fdf5e6f7db6..2a9e0545ced00 100644 --- a/src/coreclr/jit/emit.cpp +++ b/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. @@ -5103,7 +5150,7 @@ unsigned emitter::getLoopSize(insGroup* igLoopHeader, unsigned maxLoopSize DEBUG loopSize += igInLoop->igSize; JITDUMP(" %s has %u bytes.", emitLabelString(igInLoop), igInLoop->igSize); - if (igInLoop->endsWithAlignInstr() /*|| (igInLoop->igFlags & IGF_REMOVED_ALIGN)*/) + 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. @@ -5189,10 +5236,6 @@ unsigned emitter::getLoopSize(insGroup* igLoopHeader, unsigned maxLoopSize DEBUG loopSize -= emitComp->opts.compJitAlignPaddingLimit; } } - else if (igInLoop->igFlags & IGF_REMOVED_ALIGN) - { - assert(!"Failed to remove 15 bytes"); - } if ((igInLoop->igLoopBackEdge == igLoopHeader) || (loopSize > maxLoopSize)) { #ifdef DEBUG @@ -5302,8 +5345,7 @@ void emitter::emitSetLoopBackEdge(BasicBlock* loopTopBlock) assert(!markedCurrLoop); // This IG should no longer contain alignment instruction - alignInstr->removeAlignFlags(); - alignInstr->idaIG->igFlags |= IGF_REMOVED_ALIGN; + emitRemoveIGAlignFlag(alignInstr); markedCurrLoop = true; JITDUMP(";; Skip alignment for current loop IG%02u ~ IG%02u because it encloses an aligned loop " @@ -5316,11 +5358,10 @@ 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(); - alignInstr->idaIG->igFlags |= IGF_REMOVED_ALIGN; + emitRemoveIGAlignFlag(alignInstr); markedLastLoop = true; JITDUMP(";; Skip alignment for aligned loop IG%02u ~ IG%02u because it encloses the current loop " @@ -5413,7 +5454,7 @@ void emitter::emitLoopAlignAdjustments() containingIG->igFlags |= IGF_UPD_ISZ; if (actualPaddingNeeded == 0) { - alignInstr->removeAlignFlags(); + emitRemoveIGAlignFlag(alignInstr); } #ifdef TARGET_XARCH diff --git a/src/coreclr/jit/emit.h b/src/coreclr/jit/emit.h index cb7f0dc65355a..b454f1f039714 100644 --- a/src/coreclr/jit/emit.h +++ b/src/coreclr/jit/emit.h @@ -277,8 +277,6 @@ struct insGroup // and the emitter should continue to track GC info as if there was no new block. #define IGF_HAS_ALIGN 0x0400 // this group contains an alignment instruction(s) at the end to align either the next // IG, or, if this IG contains with an unconditional branch, some subsequent IG. -#define IGF_REMOVED_ALIGN 0x0800 // this group had an alignment instruction(s) which was later removed without updating - // the IG's size/offsets. // Mask of IGF_* flags that should be propagated to new blocks when they are created. // This allows prologs and epilogs to be any number of IGs, but still be @@ -1994,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. @@ -2003,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)); From d4b48e3848a6c0ec4c92b597555695db299d801e Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Thu, 5 May 2022 21:16:09 -0700 Subject: [PATCH 6/8] improve loops natvis --- src/coreclr/jit/clrjit.natvis | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/clrjit.natvis b/src/coreclr/jit/clrjit.natvis index 69e9b24f56969..e3f437c7c2ad5 100644 --- a/src/coreclr/jit/clrjit.natvis +++ b/src/coreclr/jit/clrjit.natvis @@ -28,8 +28,8 @@ Documentation for VS debugger format specifiers: https://docs.microsoft.com/en-u REMOVED - BB{lpTop->bbNum,d}-BB{lpBottom->bbNum,d} pre-h:BB{lpHead->bbNum,d} e:BB{lpEntry->bbNum,d} {lpFlags,en} - BB{lpTop->bbNum,d}-BB{lpBottom->bbNum,d} h:BB{lpHead->bbNum,d} e:BB{lpEntry->bbNum,d} {lpFlags,en} + BB{lpTop->bbNum,d}-BB{lpBottom->bbNum,d} pre-h:BB{lpHead->bbNum,d} e:BB{lpEntry->bbNum,d} [BB{lpTop->bbNum,d}..BB{lpBottom->bbNum,d}] {lpFlags,en} + BB{lpTop->bbNum,d}-BB{lpBottom->bbNum,d} h:BB{lpHead->bbNum,d} e:BB{lpEntry->bbNum,d} [BB{lpTop->bbNum,d}..BB{lpBottom->bbNum,d}] {lpFlags,en} From 15b3556014a6d5553ebc07a95fd0d6f12c5e6c66 Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Mon, 9 May 2022 11:16:04 -0700 Subject: [PATCH 7/8] Add back IGF_REMOVED_ALIGN --- src/coreclr/jit/compiler.cpp | 7 ++--- src/coreclr/jit/emit.cpp | 59 ++++-------------------------------- src/coreclr/jit/emit.h | 17 +++++++++-- 3 files changed, 23 insertions(+), 60 deletions(-) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index c0fdafef04da3..7ef8eb568633e 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -5142,7 +5142,6 @@ void Compiler::placeLoopAlignInstructions() return; } - int loopsToProcess = loopAlignCandidates; JITDUMP("Inside placeLoopAlignInstructions for %d loops.\n", loopAlignCandidates); // Add align only if there were any loops that needed alignment @@ -5154,11 +5153,11 @@ 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--; + fgFirstBB->unmarkLoopAlign(this DEBUG_ARG("prolog block")); } + int loopsToProcess = loopAlignCandidates; + for (BasicBlock* const block : Blocks()) { if (currentAlignedLoopNum != BasicBlock::NOT_IN_LOOP) diff --git a/src/coreclr/jit/emit.cpp b/src/coreclr/jit/emit.cpp index 2a9e0545ced00..a02c85ab92624 100644 --- a/src/coreclr/jit/emit.cpp +++ b/src/coreclr/jit/emit.cpp @@ -1040,7 +1040,6 @@ 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) */ @@ -5079,52 +5078,6 @@ 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. @@ -5143,14 +5096,14 @@ unsigned emitter::getLoopSize(insGroup* igLoopHeader, unsigned maxLoopSize DEBUG { unsigned loopSize = 0; - JITDUMP("*************** In getLoopSize() for %s\n", emitLabelString(igLoopHeader), loopSize); + JITDUMP("*************** In getLoopSize() for %s\n", emitLabelString(igLoopHeader)); for (insGroup* igInLoop = igLoopHeader; igInLoop != nullptr; igInLoop = igInLoop->igNext) { loopSize += igInLoop->igSize; JITDUMP(" %s has %u bytes.", emitLabelString(igInLoop), igInLoop->igSize); - if (igInLoop->endsWithAlignInstr() || emitIGHadAlignFlag(igInLoop)) + if (igInLoop->endsWithAlignInstr() || igInLoop->hadAlignInstr()) { // If IGF_HAS_ALIGN is present, igInLoop contains align instruction at the end, // for next IG or some future IG. @@ -5345,7 +5298,7 @@ void emitter::emitSetLoopBackEdge(BasicBlock* loopTopBlock) assert(!markedCurrLoop); // This IG should no longer contain alignment instruction - emitRemoveIGAlignFlag(alignInstr); + alignInstr->removeAlignFlags(); markedCurrLoop = true; JITDUMP(";; Skip alignment for current loop IG%02u ~ IG%02u because it encloses an aligned loop " @@ -5358,10 +5311,10 @@ void emitter::emitSetLoopBackEdge(BasicBlock* loopTopBlock) if (!alignLastLoop && (loopHeadIG != nullptr) && (loopHeadIG->igNum == emitLastLoopStart)) { assert(!markedLastLoop); - assert(alignInstr->idaIG->endsWithAlignInstr() || emitIGHadAlignFlag(alignInstr->idaIG)); + assert(alignInstr->idaIG->endsWithAlignInstr() || alignInstr->idaIG->hadAlignInstr()); // This IG should no longer contain alignment instruction - emitRemoveIGAlignFlag(alignInstr); + alignInstr->removeAlignFlags(); markedLastLoop = true; JITDUMP(";; Skip alignment for aligned loop IG%02u ~ IG%02u because it encloses the current loop " @@ -5454,7 +5407,7 @@ void emitter::emitLoopAlignAdjustments() containingIG->igFlags |= IGF_UPD_ISZ; if (actualPaddingNeeded == 0) { - emitRemoveIGAlignFlag(alignInstr); + alignInstr->removeAlignFlags(); } #ifdef TARGET_XARCH diff --git a/src/coreclr/jit/emit.h b/src/coreclr/jit/emit.h index b454f1f039714..b8c5342bbb033 100644 --- a/src/coreclr/jit/emit.h +++ b/src/coreclr/jit/emit.h @@ -277,6 +277,8 @@ struct insGroup // and the emitter should continue to track GC info as if there was no new block. #define IGF_HAS_ALIGN 0x0400 // this group contains an alignment instruction(s) at the end to align either the next // IG, or, if this IG contains with an unconditional branch, some subsequent IG. +#define IGF_REMOVED_ALIGN 0x0800 // IG was marked as having an alignment instruction(s), but was later unmarked + // without updating the IG's size/offsets. // Mask of IGF_* flags that should be propagated to new blocks when they are created. // This allows prologs and epilogs to be any number of IGs, but still be @@ -354,6 +356,17 @@ struct insGroup return (igFlags & IGF_HAS_ALIGN) != 0; } + // hadAlignInstr: Checks if this IG was ever 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. + bool hadAlignInstr() const + { + return (igFlags & IGF_REMOVED_ALIGN) != 0; + } + }; // end of struct insGroup // For AMD64 the maximum prolog/epilog size supported on the OS is 256 bytes @@ -1553,6 +1566,7 @@ class emitter void removeAlignFlags() { idaIG->igFlags &= ~IGF_HAS_ALIGN; + idaIG->igFlags |= IGF_REMOVED_ALIGN; } }; void emitCheckAlignFitInCurIG(unsigned nAlignInstr); @@ -1992,7 +2006,6 @@ 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. @@ -2002,8 +2015,6 @@ 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)); From 45225b497e42ecf03f40a2c9f6ab36fbf68db848 Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Tue, 10 May 2022 12:11:55 -0700 Subject: [PATCH 8/8] review comments --- src/coreclr/jit/clrjit.natvis | 4 ++-- src/coreclr/jit/emit.h | 5 ++--- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/coreclr/jit/clrjit.natvis b/src/coreclr/jit/clrjit.natvis index e3f437c7c2ad5..d3eee9af9dcaa 100644 --- a/src/coreclr/jit/clrjit.natvis +++ b/src/coreclr/jit/clrjit.natvis @@ -28,8 +28,8 @@ Documentation for VS debugger format specifiers: https://docs.microsoft.com/en-u REMOVED - BB{lpTop->bbNum,d}-BB{lpBottom->bbNum,d} pre-h:BB{lpHead->bbNum,d} e:BB{lpEntry->bbNum,d} [BB{lpTop->bbNum,d}..BB{lpBottom->bbNum,d}] {lpFlags,en} - BB{lpTop->bbNum,d}-BB{lpBottom->bbNum,d} h:BB{lpHead->bbNum,d} e:BB{lpEntry->bbNum,d} [BB{lpTop->bbNum,d}..BB{lpBottom->bbNum,d}] {lpFlags,en} + [BB{lpTop->bbNum,d}..BB{lpBottom->bbNum,d}] pre-h:BB{lpHead->bbNum,d} e:BB{lpEntry->bbNum,d} {lpFlags,en} + [BB{lpTop->bbNum,d}..BB{lpBottom->bbNum,d}] h:BB{lpHead->bbNum,d} e:BB{lpEntry->bbNum,d} {lpFlags,en} diff --git a/src/coreclr/jit/emit.h b/src/coreclr/jit/emit.h index b8c5342bbb033..ed041522613bc 100644 --- a/src/coreclr/jit/emit.h +++ b/src/coreclr/jit/emit.h @@ -359,9 +359,8 @@ struct insGroup // hadAlignInstr: Checks if this IG was ever 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. + // This method is used during loopSize calculation, where we adjust + // the loop size by removed alignment bytes. bool hadAlignInstr() const { return (igFlags & IGF_REMOVED_ALIGN) != 0;