Unify handling of constant and not constant chars during string concat

[AlekseyTs]

No description provided.

[DoctorKrolic]

I still don't understand why you want to unify lowering towards span-based approach instead of turning constant chars into strings and lower them using normal string.Concat. In fact, this has already worked that way before all that saga with spans started

[AlekseyTs]

I still don't understand why you want to unify lowering towards span-based approach instead of turning constant chars into strings and lower them using normal string.Concat

Because I do not think there is a good reason to deviate based on whether a char is a constant or not. If we think the span-based approach is good enough for non-constant chars, it should be good enough for constants as well. If it is not, then I am questioning why bother with span-based approach at all.

[DoctorKrolic]

If it is not, then I am questioning why bother with span-based approach at all.

The primary benifit of span-based approach is avoiding intermediate allocation during char -> string transformation. As a tradeoff the size of IL increases. With constant char there is no intermediate allocation because we can do transformation at compile time. So by applying span-based approach there we still pay the increased IL size, but the primary benifit is no longer there.
I understand that since you demand this I'll have to deliver it. What I still don't get is why you prefer such little consistency so much over IL size savings (simetimes quite significant as in example below) + little runtime perf boost. I mean, aren't constants always somewhat special from the compiler's point of view: they can be inlined/folded/rearranged just purely based on the fact, that they are known values without evaluation side effects, so it is somewhat natural to expect that emit strategy can differ between non-constant and constant data?

[AlekseyTs]

I understand that since you demand this I'll have to deliver it.

That is not a demand, but a suggestion. And it is based on my personal opinion. Other team members might have different opinion. And you can have different opinion as well.

[AlekseyTs]

so it is somewhat natural to expect that emit strategy can differ between non-constant and constant data?

Yes, but each case is different.

[AlekseyTs]

How about the state at commit 48 as a compromise?

[DoctorKrolic]

Here is a good concrete example (body of M2 is s + 'c'.ToString() + s + s). Code size increased from 14 to 34, which is 2.4 times increase. At the same time benchmarks show that this not only didn't improve perf, but actually made it worse:

BenchmarkDotNet v0.13.12, Windows 11 (10.0.22621.3007/22H2/2022Update/SunValley2)
AMD Ryzen 7 5800X, 1 CPU, 16 logical and 8 physical cores
.NET SDK 8.0.200
  [Host]     : .NET 8.0.2 (8.0.224.6711), X64 RyuJIT AVX2
  DefaultJob : .NET 8.0.2 (8.0.224.6711), X64 RyuJIT AVX2


| Method        | Length | Mean      | Error    | StdDev   | Gen0   | Allocated |
|-------------- |------- |----------:|---------:|---------:|-------:|----------:|
| ConcatStrings | 1      |  11.13 ns | 0.073 ns | 0.065 ns | 0.0019 |      32 B |
| ConcatSpans   | 1      |  12.42 ns | 0.054 ns | 0.050 ns | 0.0019 |      32 B |
| ConcatStrings | 1000   | 178.33 ns | 3.058 ns | 2.860 ns | 0.3600 |    6024 B |
| ConcatSpans   | 1000   | 184.59 ns | 3.651 ns | 3.237 ns | 0.3600 |    6024 B |

using BenchmarkDotNet.Attributes;
using BenchmarkDotNet.Running;

BenchmarkRunner.Run<Benchmarks>();

[MemoryDiagnoser]
public class Benchmarks
{
    private string _s;

    [Params(1, 1000)]
    public int Length { get; set; }

    [GlobalSetup]
    public void Setup() => _s = new string('s', Length);

    [Benchmark]
    public string ConcatStrings() => M1(_s);

    [Benchmark]
    public string ConcatSpans() => M2(_s);

    private static string M1(string s) => string.Concat(s, "c", s, s);

    private static string M2(string s)
    {
        var c = 'c';
        return string.Concat(s, new ReadOnlySpan<char>(in c), s, s);
    }
}

As you can see, span-based approach is slightly worse for short string length and the gap is getting larger as this length increases, so it doesn't seem like statistical noise and span codegen is clearly less efficient than string one. If we go back to the original issue the purpose of span-based concat was to avoid an intermediate allocation of unknownChar.ToString(), which slowed things down and added preasure to the GC. This is not a problem if we lower constant char to a constant string since constant string literals are interned by the runtime, so they are not allocated every time code is executed. Therefore it seems wasteful for me to emit more IL to get worse runtime performance and add unnecessary work to the JIT (since more IL corresponds to more time, required to compile it)