Use a simple temp instead of InlineArray1 #73086

RikkiGibson · 2024-04-18T19:10:00Z

jaredpar · 2024-04-19T14:30:41Z

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

+                    .Construct([elementType]);
+                var constructor = spanRefConstructor.AsMember(spanType);
+                var element = VisitExpression((BoundExpression)elements[0]);
+                var temp = _factory.StoreToTemp(element, out var assignment);


What is the re-use policies on these temps? Basically is there any chance that the temp slot allocated here will be re-used or is it considered a temp that lives for the lifetime of the current method?

I think the local slot can be reused outside the containing block, but that's fine, since the span value that references the temp can't escape outside the containing block.

Hmm. Some of our temp are statement level. I agree block level temp is fine but we should be sure which this is. Had other bugs with statement temps being reused in span before

I added a test. It doesn't look like the temp is reused.

Not sure the test is sufficient here. Looking at the code it seems that it's a re-usable temp. The default kind of the temp is SynthesizedLocalKind.LoweringTemp and that is not a long lived temp. The doc mentions these cannot live across a statement boundary

/// 1) Short-lived (temporary) /// The lifespan of a temporary variable shall not cross a statement boundary (a PDB sequence point). /// These variables are not tracked by EnC and don't have names. Only values less than 0 are considered /// short-lived: new short-lived kinds should have a negative value.

What is the re-use policies on these temps?

Slots can be reused for locals that go out of scope. The scope is defined by blocks and sequences that list locals they own. If I remember correctly, sometimes scope of locals is extended by codegen, when, for example, a sequence returns a ref to a local that it owns. One might say the bound tree violates scoping rules in such cases, but for whatever reason a decision was made to handle the case instead of enforcing correctness of the tree.

That said, symbols for temps are never reused by default. One might, of course, intentionally create a bound tree that shares the same temp for different purposes.

I hope that helps with concerns that prompted the original question.

Regarding the short-lived/long-lived story. According to my understanding, these are mostly about PDB/ENC, and the statement boundary the comment is talking about is in terms of syntax (perhaps talking in terms of sequence point boundaries would be more accurate), not about bound statement nodes. Reuse of slots in IL is not based on that. It is based on what I said in the previous message on this thread. So as long as the local is added to the right BoundBlock/BoundSequence, it will not be reused while code for that node is emitted.

There is, however, something interesting going on with where we put inline array locals. The line locals.Add(inlineArrayLocal.LocalSymbol); below. According to comments on _additionalLocals field, the local is going to end up on the enclosing method outermost block. Hopefully that is not going to mess with EnC too much because effectively the local crosses a sequence point boundary. "Collection expressions" devs might want to take a close look at this.

Great. Thansk for the explanation!

There is, however, something interesting going on with where we put inline array locals. ...

Logged #73246 based on this comment and offline discussion.

…his method is doing it

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

RikkiGibson · 2024-04-22T19:28:20Z

@dotnet/roslyn-compiler for a second review

jjonescz · 2024-04-23T08:44:19Z

src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs

+                            Console.Write(y[0]);
+                        }
+                        {
+                            Span<int> y = [x];


Do we have a test where we create single-element collection twice in the same block (not different blocks like in this test)?

Indeed this is the type of test that I was most worried about. Re-using the temp between two different blocks is just fine. Re-using it within the same block would be a significant codegen error. Want to make sure that we're not doing that.

AlekseyTs · 2024-04-25T19:00:01Z

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

+                    .Construct([elementType]);
+                var constructor = spanRefConstructor.AsMember(spanType);
+                var element = VisitExpression((BoundExpression)elements[0]);
+                var temp = _factory.StoreToTemp(element, out var assignment, isKnownToReferToTempIfReferenceType: true);


isKnownToReferToTempIfReferenceType

In general, when not sure, the safest value for this parameter is false. It enables some optimizations and you don't want to apply those when they are not appropriate because that can lead to an incorrect result, i.e. bugs.

This value is meaningful only for ref locals. I would avoid setting it to true for non-ref locals.

The meaning of this flag - this local refers to something (it is a ref), and if the type of that something is a reference type, there is a guarantee that that something is a temporary (not a user local, not a parameter, not a field, etc.) So, it is about what we are referring to with this local. Obviously the temp we are assigning to here is a temp, but this value is not about that.

So, I recommend, cleaning up this code and any other "collection expressions" code with respect to values used for this parameter. In order to avoid spreading questionable code through copy/paste and eventually hitting a scenario that can actually break because we haven't given much thought about the value. #Closed

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

jaredpar · 2024-04-26T19:30:05Z

src/Compilers/CSharp/Test/Emit2/Semantics/CollectionExpressionTests.cs

+                            Console.Write(y[0]);
+                        }
+                        {
+                            Span<int> y = [x];


Indeed this is the type of test that I was most worried about. Re-using the temp between two different blocks is just fine. Re-using it within the same block would be a significant codegen error. Want to make sure that we're not doing that.

jaredpar · 2024-04-26T19:33:01Z

src/Compilers/CSharp/Test/Emit2/Semantics/ParamsCollectionTests.cs

-                verify: ExecutionConditionUtil.IsMonoOrCoreClr ?
-                            Verification.FailsILVerify with { ILVerifyMessage = "[InlineArrayAsSpan]: Return type is ByRef, TypedReference, ArgHandle, or ArgIterator. { Offset = 0xc }" }
-                            : Verification.Skipped,
+                verify: Verification.Skipped,


Curious now. Lets say I have the following code:

M(1); M(2); void M(params Span<int> i) { }

After this change the compiler will optimize the calling code to roughly be:

int tmp = 1; M(new Span<int>(ref temp));

But do we create 1 or 2 temps here? Guessing we only create one today but want to check. This could be a significant optimization opportunity for us in the future.

jaredpar · 2024-04-26T19:35:02Z

src/Compilers/Core/Portable/WellKnownMembers.cs

+                 0,                                                                                                                                             // Arity
+                     1,                                                                                                                                         // Method Signature
+                     (byte)SignatureTypeCode.TypeHandle, (byte)SpecialType.System_Void, // Return Type
+                     (byte)SignatureTypeCode.ByReference, (byte)SignatureTypeCode.GenericTypeParameter, 0,


The signature of the ReadOnlySpan consructor changed between net7.0 and net8.0 from in to ref readonly. Will this match both versions?

In practice I expect it will. This is because the comparison here is just checking that it is a managed reference according to the runtime. It doesn't check anything about the readability or writability of the reference (C#-level concepts).

However for the particular way this optimization is implemented, I think won't even enter the path for using the 'Span referencing a temp' optimization for net7, because the containing code path is gated on InlineArray runtime feature availability, IIRC. This is shown in the codegen for the tests which use target framework net7.

Yep. I'm less interested in the optimization being applied and more interested in making sure that this well known member can see both variations of the signature. Otherwise it could create confusion for later changes.

RikkiGibson · 2024-04-29T19:05:29Z

@dotnet/roslyn-compiler for second review

jaredpar · 2024-04-29T21:24:36Z

src/Compilers/CSharp/Test/Emit2/Semantics/ParamsCollectionTests.cs

@@ -359,6 +359,77 @@ void assertAttributeData(string name)
            }
        }

+        [Fact]
+        public void Span_SingleElement_TempsAreNotReused()


Did we file a bug to follow up on this? This is a place where future optimizations could very much benefit.

In this particular case the win is pretty minor. It's just an extra int or two on the stack. Consider though when we're passing instead 3 parameters and storing lots of redundant inline array temporaries. That can start to make the stack frame size increase be noticable and may deserve an optimization pass to do proper reuse.

RikkiGibson · 2024-05-01T23:00:20Z

@dotnet/roslyn-compiler for second review

Use a simple temp instead of InlineArray1

5ee9248

dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Apr 18, 2024

RikkiGibson added 2 commits April 18, 2024 12:56

fix test failures

3e51b81

fix tests

60d90aa

RikkiGibson marked this pull request as ready for review April 18, 2024 21:23

RikkiGibson requested a review from a team as a code owner April 18, 2024 21:23

jaredpar reviewed Apr 19, 2024

View reviewed changes

Pass isKnownToReferToTempIfReferenceType because the other place in t…

9d69fa7

…his method is doing it

RikkiGibson commented Apr 19, 2024

View reviewed changes

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

RikkiGibson added 2 commits April 19, 2024 15:58

Show that temps are not reused

754c7c4

Undo unintentional change

e9e7d2a

This comment was marked as resolved.

Sign in to view

cston approved these changes Apr 20, 2024

View reviewed changes

RikkiGibson requested a review from a team April 22, 2024 19:28

jjonescz reviewed Apr 23, 2024

View reviewed changes

This comment was marked as resolved.

Sign in to view

AlekseyTs reviewed Apr 25, 2024

View reviewed changes

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

This comment was marked as resolved.

Sign in to view

cston mentioned this pull request Apr 26, 2024

Temporaries generated by LocalRewriter for collection expression spans should be marked as long-lived #73246

Open

jaredpar reviewed Apr 26, 2024

View reviewed changes

RikkiGibson added 2 commits April 26, 2024 13:19

Remove true argument for isKnownToReferToTempIfReferenceType

410ec68

Address feedback

35c2769

cston approved these changes Apr 26, 2024

View reviewed changes

RikkiGibson added 2 commits April 26, 2024 16:39

Fix test

8712cd2

fix test

1c459b3

cston approved these changes Apr 29, 2024

View reviewed changes

jaredpar reviewed Apr 29, 2024

View reviewed changes

RikkiGibson mentioned this pull request Apr 29, 2024

Collection expression span optimization should reuse temps when possible #73269

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use a simple temp instead of InlineArray1 #73086

Use a simple temp instead of InlineArray1 #73086

RikkiGibson commented Apr 18, 2024

jaredpar Apr 19, 2024

RikkiGibson Apr 19, 2024

jaredpar Apr 19, 2024

RikkiGibson Apr 19, 2024

jaredpar Apr 23, 2024

AlekseyTs Apr 25, 2024

AlekseyTs Apr 25, 2024

jaredpar Apr 25, 2024

cston Apr 26, 2024

This comment was marked as resolved.

This comment was marked as resolved.

RikkiGibson commented Apr 22, 2024

jjonescz Apr 23, 2024

jaredpar Apr 26, 2024

This comment was marked as resolved.

AlekseyTs Apr 25, 2024 •

edited

This comment was marked as resolved.

jaredpar Apr 26, 2024

jaredpar Apr 26, 2024

jaredpar Apr 26, 2024

RikkiGibson Apr 26, 2024 •

edited

jaredpar Apr 26, 2024

RikkiGibson commented Apr 29, 2024

jaredpar Apr 29, 2024

RikkiGibson Apr 29, 2024

RikkiGibson commented May 1, 2024

Use a simple temp instead of InlineArray1 #73086

Are you sure you want to change the base?

Use a simple temp instead of InlineArray1 #73086

Conversation

RikkiGibson commented Apr 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

RikkiGibson commented Apr 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

AlekseyTs Apr 25, 2024 • edited

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson Apr 26, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson commented Apr 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RikkiGibson commented May 1, 2024

AlekseyTs Apr 25, 2024 •

edited

RikkiGibson Apr 26, 2024 •

edited