Rewrite string.Concat lowering code #76955

333fred · 2025-01-28T22:57:25Z

Our existing lowering strategy for string addition is complex, to say the least. It works in a bottom-up fashion, needing to constantly undo and redo existing work as VisitExpression returns and discovered ever-larger sequences of string additions. This is not easily maintainable, and also makes addressing issues like #74538 extremely difficult. As a first step in that direction, I've rewritten how we lower these to instead approach it top-down; when a + that operates on strings is encountered, we gather all possible operands up front, fold as many constant components as we can, and then emit the final string.Concat call all in one go. This should hopefully be significantly easier to maintain in the future, as well as be a much more flexible base for building further optimizations on, such as using string.Concat(ReadOnlySpan<string>) or DefaultInterpolatedStringHandler in the future.

…t to do about how we optimize manually-written `string.Concat() + string.Concat()` calls.

333fred · 2025-01-30T22:52:17Z

src/Compilers/CSharp/Test/Emit2/CodeGen/CodeGenSpanBasedStringConcatTests.cs

-              IL_0005:  newobj     "System.ReadOnlySpan<char>..ctor(ref readonly char)"
+                            char V_1)
+              IL_0000:  ldstr      "a"
+              IL_0005:  call       "System.ReadOnlySpan<char> string.op_Implicit(string)"


This flip is going from making a ROS from a single character to loading a constant single-character string and converting it to a ROS. Talking with Stephen Toub, we expect it to be a neutral change; I can make the new code use the character-based path, but that seemed like more code to maintain for no real benefit.

333fred · 2025-01-30T23:07:51Z

src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenDynamicTests.cs

@@ -1370,24 +1370,24 @@ .locals init (C.<>c__DisplayClass11_0 V_0, //CS$<>8__locals0
  IL_02b5:  ldloc.0
  IL_02b6:  ldfld      ""dynamic C.<>c__DisplayClass11_0.dyn""
  IL_02bb:  stloc.2
-  IL_02bc:  ldsfld     ""System.Runtime.CompilerServices.CallSite<System.Func<System.Runtime.CompilerServices.CallSite, object, bool>> C.<>o__11.<>p__12""
+  IL_02bc:  ldsfld     ""System.Runtime.CompilerServices.CallSite<System.Func<System.Runtime.CompilerServices.CallSite, object, bool>> C.<>o__11.<>p__9""


The changes in this file are just related to the different visit order in the local rewriter changing the order of the fields in the display class. I've manually verified the type IL before and after.

333fred · 2025-01-30T23:10:47Z

src/Compilers/CSharp/Test/Emit2/CodeGen/CodeGenSpanBasedStringConcatTests.cs

-            //     static string M2(string s, char c) => c + s;
-            Diagnostic(ErrorCode.ERR_MissingPredefinedMember, "c").WithArguments("System.Object", "ToString").WithLocation(14, 43));
+        // We don't use object.ToString() or char.ToString() in the final codegen.
+        var verifier = CompileAndVerify(comp, expectedOutput: ExecutionConditionUtil.IsCoreClr ? "sccs" : null, verify: Verification.FailsPEVerify);


For these missing ToString tests, it didn't seem important to me to preserve the existing behavior. The existing behavior was simply a factor of the incremental build-up process of the local rewriter, and while I do think it is good to verify our behavior with non-compliant standard libraries, I don't think it matters that we ensure an error is produced here. If we break it again in some future refactoring, oh well.

333fred · 2025-01-30T23:45:28Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringConcat.cs

I would not try and review diffs here. Treat the old and new code as completely separate; the old code can be used as a reference, but the changes are too extensive to try and actually compare line-by-line.

333fred · 2025-01-30T23:48:10Z

@dotnet/roslyn-compiler, this is ready for review.

333fred · 2025-02-03T18:49:20Z

@dotnet/roslyn-compiler for reviews.

src/Compilers/Core/Portable/Collections/Rope.cs

src/Compilers/CSharp/Test/Symbol/Symbols/MissingSpecialMember.cs

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_BinaryOperator.cs

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringConcat.cs

AlekseyTs · 2025-02-04T02:20:20Z

    private BoundExpression ConvertConcatExprToString(BoundExpression expr)

Are all code paths in this helper still relevant? #Closed

Refers to: src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringConcat.cs:566 in 849602a. [](commit_id = 849602a, deletion_comment = False)

AlekseyTs · 2025-02-04T02:21:34Z

Done with review pass (commit 8)

333fred · 2025-02-04T22:56:34Z

@AlekseyTs I believe I've addressed your feedback, thanks.

AlekseyTs

LGTM (commit 10)

333fred · 2025-02-05T06:06:36Z

@dotnet/roslyn-compiler for a second review.

AlekseyTs · 2025-02-05T14:22:31Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_BinaryOperator.cs

@@ -208,7 +214,7 @@ private BoundExpression MakeBinaryOperator(
                    case BinaryOperatorKind.ObjectAndStringConcatenation:
                    case BinaryOperatorKind.StringAndObjectConcatenation:
                    case BinaryOperatorKind.StringConcatenation:
-                        return RewriteStringConcatenation(syntax, operatorKind, loweredLeft, loweredRight, type);
+                        throw ExceptionUtilities.UnexpectedValue(operatorKind);


throw ExceptionUtilities.UnexpectedValue(operatorKind);

It looks like MakeBinaryOperator helper is used in many places. Would it make sense to adjust the code to handle this code path as a stand-alone operator?

It could be, but I don't think we need to do so right now. The code that I've written presumes that the input has not yet been visited, and MakeBinaryOperator is only called on already-visited operators. While we could update the new code to accept both pre-visited and post-visited operations, I think we can save that exercise for when we encounter a case that needs them.

AlekseyTs · 2025-02-05T14:23:04Z

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_BinaryOperator.cs

@@ -256,7 +262,7 @@ private BoundExpression MakeBinaryOperator(
                    case BinaryOperatorKind.ObjectAndStringConcatenation:
                    case BinaryOperatorKind.StringAndObjectConcatenation:
                    case BinaryOperatorKind.StringConcatenation:
-                        return RewriteStringConcatenation(syntax, operatorKind, loweredLeft, loweredRight, type);
+                        throw ExceptionUtilities.UnexpectedValue(operatorKind);


throw ExceptionUtilities.UnexpectedValue(operatorKind);

Similar question here.

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringConcat.cs

jcouv

LGTM Thanks (iteration 11)

333fred added 3 commits January 24, 2025 18:00

Progress on simplifying how we lower concat. Still need to decide wha…

b33ac16

…t to do about how we optimize manually-written `string.Concat() + string.Concat()` calls.

Support extracting manually-written concat arguments

6c5221b

Support compound assignments, remove old extraction-based code.

b89469b

dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jan 28, 2025

333fred added 4 commits January 29, 2025 15:51

Last bits of adjustment

8b5c705

Do not double visit

c3c628b

Skip PEVerify

7544fe4

Skip execution on framework

d794559

333fred commented Jan 30, 2025

View reviewed changes

Ensure that direct children of a compound addition are optimized

849602a

333fred commented Jan 30, 2025

View reviewed changes

333fred marked this pull request as ready for review January 30, 2025 23:47

333fred requested a review from a team as a code owner January 30, 2025 23:47

AlekseyTs reviewed Feb 3, 2025

View reviewed changes

src/Compilers/Core/Portable/Collections/Rope.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Feb 3, 2025

View reviewed changes

src/Compilers/Core/Portable/Collections/Rope.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Feb 3, 2025

View reviewed changes

src/Compilers/CSharp/Test/Symbol/Symbols/MissingSpecialMember.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Feb 3, 2025

View reviewed changes

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_BinaryOperator.cs Show resolved Hide resolved

AlekseyTs reviewed Feb 4, 2025

View reviewed changes

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringConcat.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Feb 4, 2025

View reviewed changes

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringConcat.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Feb 4, 2025

View reviewed changes

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringConcat.cs Show resolved Hide resolved

AlekseyTs reviewed Feb 4, 2025

View reviewed changes

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringConcat.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Feb 4, 2025

View reviewed changes

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringConcat.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Feb 4, 2025

View reviewed changes

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringConcat.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Feb 4, 2025

View reviewed changes

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringConcat.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Feb 4, 2025

View reviewed changes

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringConcat.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Feb 4, 2025

View reviewed changes

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringConcat.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Feb 4, 2025

View reviewed changes

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringConcat.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Feb 4, 2025

View reviewed changes

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringConcat.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Feb 4, 2025

View reviewed changes

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringConcat.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Feb 4, 2025

View reviewed changes

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringConcat.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Feb 4, 2025

View reviewed changes

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringConcat.cs Outdated Show resolved Hide resolved

AlekseyTs reviewed Feb 4, 2025

View reviewed changes

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringConcat.cs Outdated Show resolved Hide resolved

333fred added 2 commits February 4, 2025 14:37

Address most PR feedback. Still a few comments left to handle.

db21a4b

Trim unneeded cases from ConvertConcatExprToString

4e3369c

AlekseyTs approved these changes Feb 4, 2025

View reviewed changes

AlekseyTs reviewed Feb 5, 2025

View reviewed changes

Address warning.

a37b249

jcouv reviewed Feb 6, 2025

View reviewed changes

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringConcat.cs Show resolved Hide resolved

jcouv reviewed Feb 6, 2025

View reviewed changes

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringConcat.cs Outdated Show resolved Hide resolved

jcouv reviewed Feb 6, 2025

View reviewed changes

src/Compilers/CSharp/Portable/Lowering/LocalRewriter/LocalRewriter_StringConcat.cs Show resolved Hide resolved

jcouv approved these changes Feb 6, 2025

View reviewed changes

jcouv self-assigned this Feb 6, 2025

Feedback

fd1f31d

333fred enabled auto-merge (squash) February 6, 2025 22:22

333fred merged commit a5ecdca into dotnet:main Feb 6, 2025
24 checks passed

dotnet-policy-service bot added this to the Next milestone Feb 6, 2025

333fred deleted the redo-concat branch February 7, 2025 00:27

dotnet-bot mentioned this pull request Feb 8, 2025

[Automated] PRs inserted in VS build main-35807.39 #77117

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite string.Concat lowering code #76955

Rewrite string.Concat lowering code #76955

333fred commented Jan 28, 2025

333fred Jan 30, 2025

333fred Jan 30, 2025

333fred Jan 30, 2025

333fred Jan 30, 2025

333fred commented Jan 30, 2025

333fred commented Feb 3, 2025

AlekseyTs commented Feb 4, 2025 •

edited

Loading

AlekseyTs commented Feb 4, 2025

333fred commented Feb 4, 2025

AlekseyTs left a comment

333fred commented Feb 5, 2025

AlekseyTs Feb 5, 2025

333fred Feb 5, 2025

AlekseyTs Feb 5, 2025

jcouv left a comment

Rewrite string.Concat lowering code #76955

Rewrite string.Concat lowering code #76955

Conversation

333fred commented Jan 28, 2025

333fred Jan 30, 2025

Choose a reason for hiding this comment

333fred Jan 30, 2025

Choose a reason for hiding this comment

333fred Jan 30, 2025

Choose a reason for hiding this comment

333fred Jan 30, 2025

Choose a reason for hiding this comment

333fred commented Jan 30, 2025

333fred commented Feb 3, 2025

AlekseyTs commented Feb 4, 2025 • edited Loading

AlekseyTs commented Feb 4, 2025

333fred commented Feb 4, 2025

AlekseyTs left a comment

Choose a reason for hiding this comment

333fred commented Feb 5, 2025

AlekseyTs Feb 5, 2025

Choose a reason for hiding this comment

333fred Feb 5, 2025

Choose a reason for hiding this comment

AlekseyTs Feb 5, 2025

Choose a reason for hiding this comment

jcouv left a comment

Choose a reason for hiding this comment

AlekseyTs commented Feb 4, 2025 •

edited

Loading