Unroll StringBuilder.Append for const string (#85894)

Co-authored-by: EgorBo <egorbo@gmail.com>

diff --git a/src/coreclr/jit/fgbasic.cpp b/src/coreclr/jit/fgbasic.cpp
index 7b19cfd..0e00991 100644 (file)
--- a/src/coreclr/jit/fgbasic.cpp
+++ b/src/coreclr/jit/fgbasic.cpp
@@ -1125,6 +1125,20 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed
                                 break;
                             }
 
+                            case NI_System_SpanHelpers_SequenceEqual:
+                            case NI_System_Buffer_Memmove:
+                            {
+                                if (FgStack::IsConstArgument(pushedStack.Top(), impInlineInfo))
+                                {
+                                    // Constant (at its call-site) argument feeds the Memmove/Memcmp length argument.
+                                    // We most likely will be able to unroll it.
+                                    // It is important to only raise this hint for constant arguments, if it's just a
+                                    // constant in the inlinee itself then we don't need to inline it for unrolling.
+                                    compInlineResult->Note(InlineObservation::CALLSITE_UNROLLABLE_MEMOP);
+                                }
+                                break;
+                            }
+
                             case NI_System_Span_get_Item:
                             case NI_System_ReadOnlySpan_get_Item:
                             {

diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp
index 3a84278..a20c8ba 100644 (file)
--- a/src/coreclr/jit/importercalls.cpp
+++ b/src/coreclr/jit/importercalls.cpp
@@ -3795,8 +3795,12 @@ GenTree* Compiler::impIntrinsic(GenTree*                newobjThis,
             case NI_System_SpanHelpers_SequenceEqual:
             case NI_System_Buffer_Memmove:
             {
-                // We'll try to unroll this in lower for constant input.
-                isSpecial = true;
+                if (sig->sigInst.methInstCount == 0)
+                {
+                    // We'll try to unroll this in lower for constant input.
+                    isSpecial = true;
+                }
+                // The generic version is also marked as [Intrinsic] just as a hint for the inliner
                 break;
             }
 

diff --git a/src/coreclr/jit/inline.def b/src/coreclr/jit/inline.def
index 06b6ae6..b918fb6 100644 (file)
--- a/src/coreclr/jit/inline.def
+++ b/src/coreclr/jit/inline.def
@@ -180,6 +180,7 @@ INLINE_OBSERVATION(FOLDABLE_EXPR,             int,    "foldable binary expressio
 INLINE_OBSERVATION(FOLDABLE_EXPR_UN,          int,    "foldable unary expression",            INFORMATION, CALLSITE)
 INLINE_OBSERVATION(FOLDABLE_BRANCH,           int,    "foldable branch",                      INFORMATION, CALLSITE)
 INLINE_OBSERVATION(FOLDABLE_SWITCH,           int,    "foldable switch",                      INFORMATION, CALLSITE)
+INLINE_OBSERVATION(UNROLLABLE_MEMOP,          int,    "unrollable memmove/memcmp",            INFORMATION, CALLSITE)
 INLINE_OBSERVATION(DIV_BY_CNS,                int,    "dividy by const",                      INFORMATION, CALLSITE)
 INLINE_OBSERVATION(CONSTANT_ARG_FEEDS_TEST,   bool,   "constant argument feeds test",         INFORMATION, CALLSITE)
 INLINE_OBSERVATION(DEPTH,                     int,    "depth",                                INFORMATION, CALLSITE)

diff --git a/src/coreclr/jit/inlinepolicy.cpp b/src/coreclr/jit/inlinepolicy.cpp
index ebba451..3d1d895 100644 (file)
--- a/src/coreclr/jit/inlinepolicy.cpp
+++ b/src/coreclr/jit/inlinepolicy.cpp
@@ -1317,6 +1317,10 @@ void ExtendedDefaultPolicy::NoteBool(InlineObservation obs, bool value)
             m_FoldableSwitch++;
             break;
 
+        case InlineObservation::CALLSITE_UNROLLABLE_MEMOP:
+            m_UnrollableMemop++;
+            break;
+
         case InlineObservation::CALLEE_HAS_SWITCH:
             m_Switch++;
             break;
@@ -1409,7 +1413,7 @@ void ExtendedDefaultPolicy::NoteInt(InlineObservation obs, int value)
                     // in prejit-root mode.
                     bbLimit += 5 + m_Switch * 10;
                 }
-                bbLimit += m_FoldableBranch + m_FoldableSwitch * 10;
+                bbLimit += m_FoldableBranch + m_FoldableSwitch * 10 + m_UnrollableMemop * 2;
 
                 if ((unsigned)value > bbLimit)
                 {
@@ -1715,6 +1719,13 @@ double ExtendedDefaultPolicy::DetermineMultiplier()
             break;
     }
 
+    if (m_UnrollableMemop > 0)
+    {
+        multiplier += m_UnrollableMemop;
+        JITDUMP("\nInline candidate has %d unrollable memory operations.  Multiplier increased to %g.",
+                m_UnrollableMemop, multiplier);
+    }
+
     if (m_FoldableSwitch > 0)
     {
         multiplier += 6.0;
@@ -1841,6 +1852,7 @@ void ExtendedDefaultPolicy::OnDumpXml(FILE* file, unsigned indent) const
     XATTR_I4(m_FoldableExprUn)
     XATTR_I4(m_FoldableBranch)
     XATTR_I4(m_FoldableSwitch)
+    XATTR_I4(m_UnrollableMemop)
     XATTR_I4(m_Switch)
     XATTR_I4(m_DivByCns)
     XATTR_B(m_ReturnsStructByValue)

diff --git a/src/coreclr/jit/inlinepolicy.h b/src/coreclr/jit/inlinepolicy.h
index cc15348..57c171f 100644 (file)
--- a/src/coreclr/jit/inlinepolicy.h
+++ b/src/coreclr/jit/inlinepolicy.h
@@ -216,6 +216,7 @@ public:
         , m_FoldableExprUn(0)
         , m_FoldableBranch(0)
         , m_FoldableSwitch(0)
+        , m_UnrollableMemop(0)
         , m_Switch(0)
         , m_DivByCns(0)
         , m_ReturnsStructByValue(false)
@@ -268,6 +269,7 @@ protected:
     unsigned m_FoldableExprUn;
     unsigned m_FoldableBranch;
     unsigned m_FoldableSwitch;
+    unsigned m_UnrollableMemop;
     unsigned m_Switch;
     unsigned m_DivByCns;
     bool     m_ReturnsStructByValue : 1;

diff --git a/src/libraries/System.Private.CoreLib/src/System/Buffer.cs b/src/libraries/System.Private.CoreLib/src/System/Buffer.cs
index 8328c18..f343c6e 100644 (file)
--- a/src/libraries/System.Private.CoreLib/src/System/Buffer.cs
+++ b/src/libraries/System.Private.CoreLib/src/System/Buffer.cs
@@ -345,6 +345,7 @@ namespace System
 
 #if !MONO // Mono BulkMoveWithWriteBarrier is in terms of elements (not bytes) and takes a type handle.
 
+        [Intrinsic]
         [MethodImpl(MethodImplOptions.AggressiveInlining)]
         internal static unsafe void Memmove<T>(ref T destination, ref T source, nuint elementCount)
         {

diff --git a/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs b/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs
index e2ea2a6..395e509 100644 (file)
--- a/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs
+++ b/src/libraries/System.Private.CoreLib/src/System/Text/StringBuilder.cs
@@ -730,7 +730,7 @@ namespace System.Text
         {
             if (value is not null)
             {
-                Append(valueCount: value.Length, value: ref value.GetRawStringData());
+                Append(ref value.GetRawStringData(), value.Length);
             }
 
             return this;
@@ -2107,7 +2107,7 @@ namespace System.Text
         /// <summary>Appends a specified number of chars starting from the specified reference.</summary>
         private void Append(ref char value, int valueCount)
         {
-            Debug.Assert(valueCount >= 0, $"Invalid length; should have been validated by caller.");
+            Debug.Assert(valueCount >= 0, "Invalid length; should have been validated by caller.");
             if (valueCount != 0)
             {
                 char[] chunkChars = m_ChunkChars;