Only compute if method returns a constant when needed

src/linker/Linker.Steps/RemoveUnreachableBlocksStep.cs

@@ -17,81 +17,61 @@ namespace Mono.Linker.Steps
 	public class RemoveUnreachableBlocksStep : BaseStep
 	{
 		Dictionary<MethodDefinition, Instruction> constExprMethods;
+		bool constExprMethodsAdded;
 		MethodDefinition IntPtrSize, UIntPtrSize;
 
 		protected override void Process ()
 		{
 			var assemblies = Context.Annotations.GetAssemblies ().ToArray ();
 
 			constExprMethods = new Dictionary<MethodDefinition, Instruction> ();
-			foreach (var assembly in assemblies) {
-				FindConstantExpressionsMethods (assembly.MainModule.Types);
-			}
-
-			if (constExprMethods.Count == 0)
-				return;
 
-			int constExprMethodsCount;
 			do {
 				//
 				// Body rewriting can produce more methods with constant expression
 				//
-				constExprMethodsCount = constExprMethods.Count;
+				constExprMethodsAdded = false;
 
 				foreach (var assembly in assemblies) {
 					if (Annotations.GetAction (assembly) != AssemblyAction.Link)
 						continue;
 
 					RewriteBodies (assembly.MainModule.Types);
 				}
-			} while (constExprMethodsCount < constExprMethods.Count);
+			} while (constExprMethodsAdded);
 		}
 
-		void FindConstantExpressionsMethods (Collection<TypeDefinition> types)
+		bool TryGetConstantResultInstructionForMethod (MethodDefinition method, out Instruction constantResultInstruction)
 		{
-			foreach (var type in types) {
-				if (type.IsInterface)
-					continue;
-
-				if (!type.HasMethods)
-					continue;
+			if (constExprMethods.TryGetValue (method, out constantResultInstruction))
+				return constantResultInstruction != null;
 
-				foreach (var method in type.Methods) {
-					if (!method.HasBody)
-						continue;
-
-					if (method.ReturnType.MetadataType == MetadataType.Void)
-						continue;
+			constantResultInstruction = GetConstantResultInstructionForMethod (method);
+			constExprMethods.Add (method, constantResultInstruction);
 
-					switch (Annotations.GetAction (method)) {
-					case MethodAction.ConvertToThrow:
-						continue;
-					case MethodAction.ConvertToStub:
-						var instruction = CodeRewriterStep.CreateConstantResultInstruction (Context, method);
-						if (instruction != null)
-							constExprMethods[method] = instruction;
-
-						continue;
-					}
+			return constantResultInstruction != null;
+		}
 
-					if (method.IsIntrinsic () || method.NoInlining)
-						continue;
+		Instruction GetConstantResultInstructionForMethod (MethodDefinition method)
+		{
+			if (!method.HasBody)
+				return null;
 
-					if (constExprMethods.ContainsKey (method))
-						continue;
+			if (method.ReturnType.MetadataType == MetadataType.Void)
+				return null;
 
-					if (!Context.IsOptimizationEnabled (CodeOptimizations.IPConstantPropagation, method))
-						continue;
+			if (method.IsIntrinsic () || method.NoInlining)
+				return null;
 
-					var analyzer = new ConstantExpressionMethodAnalyzer (method);
-					if (analyzer.Analyze ()) {
-						constExprMethods[method] = analyzer.Result;
-					}
-				}
+			if (!Context.IsOptimizationEnabled (CodeOptimizations.IPConstantPropagation, method))
+				return null;
 
-				if (type.HasNestedTypes)
-					FindConstantExpressionsMethods (type.NestedTypes);
+			var analyzer = new ConstantExpressionMethodAnalyzer (Context, method);
+			if (analyzer.Analyze ()) {
+				return analyzer.Result;
 			}
+
+			return null;
 		}
 
 		void RewriteBodies (Collection<TypeDefinition> types)
@@ -115,7 +95,6 @@ void RewriteBodies (Collection<TypeDefinition> types)
 					case MetadataType.FunctionPointer:
 						continue;
 					}
-
 					RewriteBody (method);
 				}
 
@@ -148,12 +127,15 @@ void RewriteBody (MethodDefinition method)
 			if (method.ReturnType.MetadataType == MetadataType.Void)
 				return;
 
-			//
-			// Re-run the analyzer in case body change rewrote it to constant expression
-			//
-			var analyzer = new ConstantExpressionMethodAnalyzer (method, reducer.FoldedInstructions);
-			if (analyzer.Analyze ()) {
-				constExprMethods[method] = analyzer.Result;
+			if (!constExprMethods.TryGetValue (method, out var constInstruction) || constInstruction == null) {
+				//
+				// Re-run the analyzer in case body change rewrote it to constant expression
+				//
+				var analyzer = new ConstantExpressionMethodAnalyzer (Context, method, reducer.FoldedInstructions);
+				if (analyzer.Analyze ()) {
+					constExprMethods[method] = analyzer.Result;
+					constExprMethodsAdded = true;
+				}
 			}
 		}
 
@@ -174,9 +156,6 @@ bool TryInlineBodyDependencies (ref BodyReducer reducer)
 					if (md == null)
 						break;
 
-					if (!constExprMethods.TryGetValue (md, out targetResult))
-						break;
-
 					if (md.CallingConvention == MethodCallingConvention.VarArg)
 						break;
 
@@ -208,8 +187,12 @@ bool TryInlineBodyDependencies (ref BodyReducer reducer)
 							break;
 					}
 
+					if (!TryGetConstantResultInstructionForMethod (md, out targetResult))
+						break;
+
 					reducer.Rewrite (i, targetResult);
 					changed = true;
+
 					break;
 
 				case Code.Ldsfld:
@@ -244,7 +227,7 @@ bool TryInlineBodyDependencies (ref BodyReducer reducer)
 						sizeOfImpl = (IntPtrSize ??= FindSizeMethod (operand.Resolve ()));
 					}
 
-					if (sizeOfImpl != null && constExprMethods.TryGetValue (sizeOfImpl, out targetResult)) {
+					if (sizeOfImpl != null && TryGetConstantResultInstructionForMethod (sizeOfImpl, out targetResult)) {
 						reducer.Rewrite (i, targetResult);
 						changed = true;
 					}
@@ -1042,34 +1025,41 @@ static bool IsSideEffectFreeLoad (Instruction instr)
 
 		struct ConstantExpressionMethodAnalyzer
 		{
+			readonly LinkContext context;
 			readonly MethodDefinition method;
 			readonly Collection<Instruction> instructions;
 
 			Stack<Instruction> stack_instr;
 			Dictionary<int, Instruction> locals;
 
-			public ConstantExpressionMethodAnalyzer (MethodDefinition method)
+			public ConstantExpressionMethodAnalyzer (LinkContext context, MethodDefinition method)
 			{
+				this.context = context;
 				this.method = method;
 				instructions = method.Body.Instructions;
 				stack_instr = null;
 				locals = null;
 				Result = null;
 			}
 
-			public ConstantExpressionMethodAnalyzer (MethodDefinition method, Collection<Instruction> instructions)
+			public ConstantExpressionMethodAnalyzer (LinkContext context, MethodDefinition method, Collection<Instruction> instructions)
+				: this (context, method)
 			{
-				this.method = method;
 				this.instructions = instructions;
-				stack_instr = null;
-				locals = null;
-				Result = null;
 			}
 
 			public Instruction Result { get; private set; }
 
 			public bool Analyze ()
 			{
+				switch (context.Annotations.GetAction (method)) {
+				case MethodAction.ConvertToThrow:
+					return false;
+				case MethodAction.ConvertToStub:
+					Result = CodeRewriterStep.CreateConstantResultInstruction (context, method);
+					return Result != null;
+				}
+
 				var body = method.Body;
 				if (body.HasExceptionHandlers)
 					return false;

test/Mono.Linker.Tests.Cases/FeatureSettings/FeatureSubstitutions.cs

@@ -6,6 +6,7 @@ namespace Mono.Linker.Tests.Cases.FeatureSettings
 {
 	[SetupLinkerSubstitutionFile ("FeatureSubstitutions.xml")]
 	[SetupLinkerArgument ("--feature", "OptionalFeature", "false")]
+	[SetupLinkerArgument ("--enable-opt", "ipconstprop")]
 	public class FeatureSubstitutions
 	{
 		[Kept]

test/Mono.Linker.Tests.Cases/UnreachableBlock/BodiesWithSubstitutions.cs

@@ -40,6 +40,7 @@ public static void Main ()
 
 			ConstantFromNewAssembly.Test ();
 			ConstantSubstitutionsFromNewAssembly.Test ();
+			TestSubstitutionCollision ();
 		}
 
 		[Kept]
@@ -246,5 +247,36 @@ public static void Test ()
 					Reached ();
 			}
 		}
+
+		[Kept]
+		static bool CollisionProperty {
+			[Kept]
+			[ExpectBodyModified]
+			get {
+				// Need to call something with constant value to force processing of this method
+				_ = Property;
+				return true;
+			} // Substitution will set this to false
+		}
+
+		// This tests that if there's a method (get_CollisionProperty) which itself is constant
+		// and substitution changes its return value, the branch removal reacts to the substituted value
+		// and not the value from the method's body.
+		// This should ideally never happen, but still.
+		// In the original code this test would be order dependent. Depending if TestSubstitutionsCollision
+		// was processed before CollisionProperty, it would either propagate true or false.
+		[Kept]
+		[ExpectBodyModified]
+		static void TestSubstitutionCollision ()
+		{
+			if (CollisionProperty)
+				Collision_NeverReached ();
+			else
+				Collision_Reached ();
+		}
+
+		[Kept]
+		static void Collision_Reached () { }
+		static void Collision_NeverReached () { }
 	}
 }

test/Mono.Linker.Tests.Cases/UnreachableBlock/BodiesWithSubstitutions.xml

@@ -3,6 +3,8 @@
     <type fullname="Mono.Linker.Tests.Cases.UnreachableBlock.BodiesWithSubstitutions">
       <method signature="System.Int32 get_Property()" body="stub" value="3">
       </method>
+      <method signature="System.Boolean get_CollisionProperty()" body="stub" value="false">
+      </method>
     </type>
     <type fullname="Mono.Linker.Tests.Cases.UnreachableBlock.BodiesWithSubstitutions/ClassWithField">
       <field name="SField" value="9"/>