Add constant propagation through methods with parameters or complex bodies #2556

marek-safar · 2022-01-28T10:46:12Z

This change also

Adds simple inlining of method return values.
Fixes several shortcomings of existing implementation like ignoring
methods called from unreachable blocks.

This helps to reduce the size of all runtime assemblies. For example size
of SPC for the default iOS project is reduced by 4% mostly due to metadata
removal. The linker speed improves a little bit (~3%) for the same
project.

MichalStrehovsky · 2022-01-28T15:15:44Z

I'm not actually reviewing this, but I had a couple areas of interest that I skimmed:

Inlining string literals across modules is observable; we avoid doing it in CoreCLR: https://github.com/dotnet/runtime/blob/017744628361a6fc45c05c75cf640292c670a8dd/src/coreclr/vm/jitinterface.cpp#L8082-L8090
Doing string concat ahead of time is observable too for similar reasons (if the newly concatenated string matches an existing literal). As the CoreCLR comment says "bugs are going to be tough to find". An example that has been brought up in the past is if someone takes a lock with such string, or uses object.ReferenceEqual because they expected a fresh string.
Is there anything to prevent removing calls to methods on types with static constructors/module constructors?
I think this is currently removing side effect for a callvirt with a null this (static void Call(Foo instance) => instance.X(); - I saw a check for ldarg_0, but I didn't see a check for whether the calling method is instance).

marek-safar · 2022-01-29T09:05:11Z

Inlining string literals across modules is observable; we avoid doing it in CoreCLR

Do you have an example why? I didn't understand the comment.

Doing string concat ahead of time is observable too for similar reasons

~~Ugh, this is quite corner case but fair enough will address that.~~

The code is not used for actually inlining only for result evaluation.

Is there anything to prevent removing calls to methods on types with static constructors/module constructors?

I had logic there but removed, now thinking about it again it needs more work even for other scenarios.

I think this is currently removing side effect for a callvirt with a null this

It does not but there is a bug in the logic (missing a special case for static context).

MichalStrehovsky · 2022-01-31T02:23:07Z

Inlining string literals across modules is observable; we avoid doing it in CoreCLR

Do you have an example why? I didn't understand the comment.

It has to do with CompilationRelaxations.NoStringInterning (that the C# compiler automatically emits into all assemblies it produces). The docs on it are pretty poor, here's StackOverflow: https://stackoverflow.com/questions/15601916/how-does-compilationrelaxations-nostringinterning-actually-work. That said, I looked into the implementation on the runtime side and at this point (after summer of last year) it seems like we no longer need to worry about it.

I created dotnet/runtime#64521 to really double check it's no longer a concern. I expect JanK to scream if it's not the case and I'm wrong.

jkotas · 2022-01-31T02:42:16Z

src/linker/Linker.Steps/UnreachableBlocksOptimizer.cs

+
+					if (left is string sleft && right is string sright) {
+						if (method.Name.Length == 6)
+							return Instruction.Create (OpCodes.Ldstr, string.Concat (sleft, sright));


This can create a very long string if called recursively.

Just curious - what are examples of CoreLib methods where this helps with trimming?

It does not matter if it creates long string as this logic is used for result computation only (value is not inlined). I found it for example here https://cs.github.com/dotnet/runtime/blob/f85ea976f81945ea18cd5dc71959cccecdc93cd2/src/libraries/System.Private.CoreLib/src/System/TimeZoneInfo.cs?q=GetUtcFullDisplayName#L2111

…dies This change also - Adds simple inlining of method return values. - Fixes several shortcomings of existing implementation like ignoring methods called from unreachable blocks. This helps to reduce the size of all runtime assemblies. For example size of SPC for the default iOS project is reduced by 4% mostly due to metadata removal. The linker speed improves a little bit (~3%) for the same project.

marek-safar · 2022-02-02T16:52:38Z

Is there anything to prevent removing calls to methods on types with static constructors/module constructors?

There is extra logic to take care of that.

I think this is currently removing side effect for a callvirt with a null this (static void Call(Foo instance) => instance.X(); - I saw a check for ldarg_0, but I didn't see a check for whether the calling method is instance).

Yep, made the check more comprehensive.

src/linker/Linker.Steps/UnreachableBlocksOptimizer.cs

vitek-karas · 2022-02-17T16:40:54Z

src/linker/Linker.Steps/UnreachableBlocksOptimizer.cs

-						sizeOfImpl = (IntPtrSize ??= FindSizeMethod (_context.TryResolve (operand)));
-					}
+						var cpl = new CalleePayload (md, GetArgumentsOnStack (md, body.Instructions, i));
+						MethodResult? call_result = optimizer.TryGetMethodCallResult (cpl, optimizer.GetRecursionGuard ());


This is a major difference in how the implementation worked before - the previous version didn't have on-the-stack recursion, it handled this by storing info on the heap. I don't remember if we ever measured the maximum depth reached, but in the previous version it would not matter all. In this version the linker could run out of stack space and hard crash.

It took some non-trivial logic to make the code work well with on-the-heap storage, so the on-the-stack version is simpler. It could be OK this way, but we should be aware of it.

At the very least we should add a hard coded constant and cap the stack depth - basically if we run out of stack linker would not crash, but it would not optimize as well as it could. Much better than hard crashing.

Added the deep stack check, the implementation goes only up to 3 iterations down on the default iOS app so I think this is fine.

vitek-karas · 2022-02-17T16:51:46Z

src/linker/Linker.Steps/UnreachableBlocksOptimizer.cs

+							}
+						}
+
+						processor.Replace (i, result.GetPrototype ());


I must admit I find the name GetPrototype pretty confusing - it's a copy (or shallow copy), why not call it that?

It can be both, shallow copy for complex instruction, deep copy for simple instruction. I have upstream PR which I'll merge here once approved (including the name)

vitek-karas · 2022-02-17T17:00:05Z

src/linker/Linker.Steps/UnreachableBlocksOptimizer.cs

+
 			[MemberNotNullWhen (true, "Result")]
-			public bool Analyze ()
+			public bool Analyze (in CalleePayload callee, Stack<MethodDefinition> callStack)


This is very similar to MethodBodyScanner - might be worth looking into potential sharing. Or at the very least comparing the two implementations.

That said - with the linker switching over to the "solver based data flow", the method body scanner will probably change quite a bit - especially around branching and such (basically it will become BasicBlockScanner instead).

src/linker/Linker.Steps/UnreachableBlocksOptimizer.cs

vitek-karas

I think this looks good now.

…odies (dotnet/linker#2556) Commit migrated from dotnet/linker@bacafd1

marek-safar changed the title ~~Add constant propagation through methods with parameter or complex bodies~~ Add constant propagation through methods with parameters or complex bodies Jan 28, 2022

marek-safar requested a review from vitek-karas January 28, 2022 10:46

marek-safar marked this pull request as draft January 29, 2022 08:59

jkotas reviewed Jan 31, 2022

View reviewed changes

marek-safar force-pushed the constprop-next branch from 2ebe241 to c59b3a0 Compare February 2, 2022 16:42

marek-safar marked this pull request as ready for review February 2, 2022 16:52

vitek-karas reviewed Feb 17, 2022

View reviewed changes

marek-safar added 3 commits March 18, 2022 18:07

PR feedback

2492a27

Merge remote-tracking branch 'upstream/main' into constprop-next

ca145a1

A bit more tweaks

00750d6

vitek-karas reviewed Mar 30, 2022

View reviewed changes

More feedback

c66e63b

vitek-karas approved these changes Mar 31, 2022

View reviewed changes

Merge branch 'main' into constprop-next

19fb2c0

marek-safar merged commit bacafd1 into dotnet:main Apr 7, 2022

marek-safar deleted the constprop-next branch April 7, 2022 12:53

eerhardt mentioned this pull request May 11, 2022

Fix linker tests not running and show full path during trimming test execution dotnet/runtime#69118

Merged

agocke pushed a commit to dotnet/runtime that referenced this pull request Nov 16, 2022

Add constant propagation through methods with parameters or complex b…

d2f47dd

…odies (dotnet/linker#2556) Commit migrated from dotnet/linker@bacafd1

Add constant propagation through methods with parameters or complex bodies #2556

Add constant propagation through methods with parameters or complex bodies #2556

Uh oh!

Conversation

marek-safar commented Jan 28, 2022

Uh oh!

MichalStrehovsky commented Jan 28, 2022

Uh oh!

marek-safar commented Jan 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MichalStrehovsky commented Jan 31, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marek-safar commented Feb 2, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vitek-karas left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

marek-safar commented Jan 29, 2022 •

edited

Loading