Fix constant prop around intrinsic (#1749)

In #1734 a change was made that substitutions are only considered for methods which are not intrinsic or no-inline. That is not how it should work, even intrinsic or no-inline methods should still allow substitutions and constant propagation from those.

What intrinsic and no-inline methods should not do is participate in constant propagation as intermediate methods (so methods which become constant because they're calling other constant methods).

In short substitutions effectively override the intrinsic/no-inline.

Added test cases.

Fixes #1747

src/linker/Linker.Steps/RemoveUnreachableBlocksStep.cs

@@ -46,27 +46,34 @@ bool TryGetConstantResultInstructionForMethod (MethodDefinition method, out Inst
 			if (constExprMethods.TryGetValue (method, out constantResultInstruction))
 				return constantResultInstruction != null;
 
-			constantResultInstruction = GetConstantResultInstructionForMethod (method);
+			constantResultInstruction = GetConstantResultInstructionForMethod (method, instructions: null);
 			constExprMethods.Add (method, constantResultInstruction);
 
 			return constantResultInstruction != null;
 		}
 
-		Instruction GetConstantResultInstructionForMethod (MethodDefinition method)
+		Instruction GetConstantResultInstructionForMethod (MethodDefinition method, Collection<Instruction> instructions)
 		{
 			if (!method.HasBody)
 				return null;
 
 			if (method.ReturnType.MetadataType == MetadataType.Void)
 				return null;
 
+			switch (Context.Annotations.GetAction (method)) {
+			case MethodAction.ConvertToThrow:
+				return null;
+			case MethodAction.ConvertToStub:
+				return CodeRewriterStep.CreateConstantResultInstruction (Context, method);
+			}
+
 			if (method.IsIntrinsic () || method.NoInlining)
 				return null;
 
 			if (!Context.IsOptimizationEnabled (CodeOptimizations.IPConstantPropagation, method))
 				return null;
 
-			var analyzer = new ConstantExpressionMethodAnalyzer (Context, method);
+			var analyzer = new ConstantExpressionMethodAnalyzer (method, instructions ?? method.Body.Instructions);
 			if (analyzer.Analyze ()) {
 				return analyzer.Result;
 			}
@@ -131,9 +138,9 @@ void RewriteBody (MethodDefinition method)
 				//
 				// 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;
+				var constantResultInstruction = GetConstantResultInstructionForMethod (method, reducer.FoldedInstructions);
+				if (constantResultInstruction != null) {
+					constExprMethods[method] = constantResultInstruction;
 					constExprMethodsAdded = true;
 				}
 			}
@@ -1025,25 +1032,23 @@ 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 (LinkContext context, MethodDefinition method)
+			public ConstantExpressionMethodAnalyzer (MethodDefinition method)
 			{
-				this.context = context;
 				this.method = method;
 				instructions = method.Body.Instructions;
 				stack_instr = null;
 				locals = null;
 				Result = null;
 			}
 
-			public ConstantExpressionMethodAnalyzer (LinkContext context, MethodDefinition method, Collection<Instruction> instructions)
-				: this (context, method)
+			public ConstantExpressionMethodAnalyzer (MethodDefinition method, Collection<Instruction> instructions)
+				: this (method)
 			{
 				this.instructions = instructions;
 			}
@@ -1052,14 +1057,6 @@ public ConstantExpressionMethodAnalyzer (LinkContext context, MethodDefinition m
 
 			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.Expectations/Support/IntrinsicAttribute.cs

@@ -0,0 +1,15 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+using System;
+
+namespace System.Runtime.CompilerServices
+{
+	// This attribute is normally implemented in CoreLib as internal, but in order to test
+	// linker behavior around it, we need to be able to use it in the tests.
+	[AttributeUsage (AttributeTargets.Method)]
+	public sealed class IntrinsicAttribute : Attribute
+	{
+	}
+}

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

@@ -17,6 +17,7 @@ namespace Mono.Linker.Tests.Cases.UnreachableBlock
 		"LibWithConstantSubstitution.dll",
 		new[] { "Dependencies/LibWithConstantSubstitution.cs" },
 		resources: new object[] { "Dependencies/LibWithConstantSubstitution.xml" })]
+	[KeptModuleReference ("unknown")]
 	public class BodiesWithSubstitutions
 	{
 		static class ClassWithField
@@ -41,6 +42,9 @@ public static void Main ()
 			ConstantFromNewAssembly.Test ();
 			ConstantSubstitutionsFromNewAssembly.Test ();
 			TestSubstitutionCollision ();
+			TestSubstitutionOnNoInlining ();
+			TestSubstitutionOnIntrinsic ();
+			TestMethodWithoutBody ();
 		}
 
 		[Kept]
@@ -278,5 +282,64 @@ static void TestSubstitutionCollision ()
 		[Kept]
 		static void Collision_Reached () { }
 		static void Collision_NeverReached () { }
+
+		[Kept]
+		static bool NoInliningProperty {
+			[Kept]
+			[ExpectBodyModified]
+			[MethodImpl (MethodImplOptions.NoInlining)]
+			get { return true; }
+		}
+
+		[Kept]
+		[ExpectBodyModified]
+		static void TestSubstitutionOnNoInlining ()
+		{
+			if (NoInliningProperty)
+				NoInlining_NeverReached ();
+			else
+				NoInlining_Reached ();
+		}
+
+		[Kept]
+		static void NoInlining_Reached () { }
+		static void NoInlining_NeverReached () { }
+
+		[Kept]
+		static bool IntrinsicProperty {
+			[Kept]
+			[ExpectBodyModified]
+			[Intrinsic]
+			[KeptAttributeAttribute (typeof (IntrinsicAttribute))]
+			get { return true; }
+		}
+
+		[Kept]
+		[ExpectBodyModified]
+		static void TestSubstitutionOnIntrinsic ()
+		{
+			if (IntrinsicProperty)
+				Intrinsic_NeverReached ();
+			else
+				Intrinsic_Reached ();
+		}
+
+		[Kept]
+		static void Intrinsic_Reached () { }
+		static void Intrinsic_NeverReached () { }
+
+		[Kept]
+		[System.Runtime.InteropServices.DllImport ("unknown")]
+		static extern int PInvokeMethod ();
+
+		[Kept]
+		static void TestMethodWithoutBody ()
+		{
+			if (PInvokeMethod () == 0)
+				MethodWithoutBody_Reached ();
+		}
+
+		[Kept]
+		static void MethodWithoutBody_Reached () { }
 	}
 }

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

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