Constant propagation directly from MarkStep

src/linker/Linker.Steps/MarkStep.cs

@@ -33,7 +33,6 @@
 using System.Diagnostics.CodeAnalysis;
 using System.Linq;
 using System.Reflection.Runtime.TypeParsing;
-using System.Runtime.CompilerServices;
 using System.Text.RegularExpressions;
 using Mono.Cecil;
 using Mono.Cecil.Cil;
@@ -57,6 +56,7 @@ public partial class MarkStep : IStep
 		protected List<MethodBody> _unreachableBodies;
 
 		readonly List<(TypeDefinition Type, MethodBody Body, Instruction Instr)> _pending_isinst_instr;
+		UnreachableBlocksOptimizer _removeUnreachableBlocksStep;
 
 #if DEBUG
 		static readonly DependencyKind[] _entireTypeReasons = new DependencyKind[] {
@@ -182,6 +182,7 @@ public MarkStep ()
 		public virtual void Process (LinkContext context)
 		{
 			_context = context;
+			_removeUnreachableBlocksStep = new UnreachableBlocksOptimizer (_context);
 
 			Initialize ();
 			Process ();
@@ -2505,6 +2506,8 @@ protected virtual void ProcessMethod (MethodDefinition method, in DependencyInfo
 				throw new ArgumentOutOfRangeException ($"Internal error: unsupported method dependency {reason.Kind}");
 #endif
 
+			_removeUnreachableBlocksStep.ProcessMethod (method);
+
 			// Record the reason for marking a method on each call. The logic under CheckProcessed happens
 			// only once per method.
 			switch (reason.Kind) {

src/linker/Linker.Steps/RemoveUnreachableBlocksStep.cs → src/linker/Linker.Steps/UnreachableBlocksOptimizer.cs

@@ -10,12 +10,13 @@
 namespace Mono.Linker.Steps
 {
 	//
-	// This steps evaluates simple properties or methods for constant expressions and
+	// Evaluates simple properties or methods for constant expressions and
 	// then uses this information to remove unreachable conditional blocks. It does
 	// not do any inlining-like code changes.
 	//
-	public class RemoveUnreachableBlocksStep : BaseStep
+	public class UnreachableBlocksOptimizer
 	{
+		readonly LinkContext _context;
 		MethodDefinition IntPtrSize, UIntPtrSize;
 
 		readonly struct ProcessingNode
@@ -39,7 +40,7 @@ public ProcessingNode (in ProcessingNode other, int newLastAttempStackVersion)
 		// Stack of method nodes which are currently being processed.
 		// Implemented as linked list to allow easy referal to nodes and efficient moving of nodes within the list.
 		// The top of the stack is the first item in the list.
-		LinkedList<ProcessingNode> processingStack;
+		readonly LinkedList<ProcessingNode> _processingStack;
 
 		// Each time an item is added or removed from the processing stack this value is incremented.
 		// Moving items in the stack doesn't increment.
@@ -50,11 +51,11 @@ public ProcessingNode (in ProcessingNode other, int newLastAttempStackVersion)
 		// if we get around to process the method again and the version of the stack didn't change, then there's a loop
 		// (nothing changed in the stack - order is unimportant, as such no new information has been added and so
 		// we can't resolve the situation with just the info at hand).
-		int processingStackVersion;
+		int _processingStackVersion;
 
 		// Just a fast lookup from method to the node on the stack. This is needed to be able to quickly
 		// access the node and move it to the top of the stack.
-		Dictionary<MethodDefinition, LinkedListNode<ProcessingNode>> processingMethods;
+		readonly Dictionary<MethodDefinition, LinkedListNode<ProcessingNode>> _processingMethods;
 
 		// Stores results of method processing. This state is kept forever to avoid reprocessing of methods.
 		// If method is not in the dictionary it has not yet been processed.
@@ -64,54 +65,17 @@ public ProcessingNode (in ProcessingNode other, int newLastAttempStackVersion)
 		//   - Instruction instance - method has been processed and it has a constant return value (the value of the instruction)
 		// Note: ProcessedUnchangedSentinel is used as an optimization. running constant value analysis on a method is relatively expensive
 		// and so we delay it and only do it for methods where the value is asked for (or in case of changed methods upfront due to implementation detailds)
-		Dictionary<MethodDefinition, Instruction> processedMethods;
+		readonly Dictionary<MethodDefinition, Instruction> _processedMethods;
 		static readonly Instruction ProcessedUnchangedSentinel = Instruction.Create (OpCodes.Ldstr, "ProcessedUnchangedSentinel");
 		static readonly Instruction NonConstSentinel = Instruction.Create (OpCodes.Ldstr, "NonConstSentinel");
 
-		protected override void Process ()
+		public UnreachableBlocksOptimizer (LinkContext context)
 		{
-			var assemblies = Context.Annotations.GetAssemblies ().ToArray ();
+			_context = context;
 
-			processingStack = new LinkedList<ProcessingNode> ();
-			processingMethods = new Dictionary<MethodDefinition, LinkedListNode<ProcessingNode>> ();
-			processedMethods = new Dictionary<MethodDefinition, Instruction> ();
-
-			foreach (var assembly in assemblies) {
-				if (Annotations.GetAction (assembly) != AssemblyAction.Link)
-					continue;
-
-				ProcessMethods (assembly.MainModule.Types);
-			}
-		}
-
-		void ProcessMethods (Collection<TypeDefinition> types)
-		{
-			foreach (var type in types) {
-				if (type.IsInterface)
-					continue;
-
-				if (!type.HasMethods)
-					continue;
-
-				foreach (var method in type.Methods) {
-					if (!method.HasBody)
-						continue;
-
-					//
-					// Block methods which rewrite does not support
-					//
-					switch (method.ReturnType.MetadataType) {
-					case MetadataType.ByReference:
-					case MetadataType.FunctionPointer:
-						continue;
-					}
-
-					ProcessMethod (method);
-				}
-
-				if (type.HasNestedTypes)
-					ProcessMethods (type.NestedTypes);
-			}
+			_processingStack = new LinkedList<ProcessingNode> ();
+			_processingMethods = new Dictionary<MethodDefinition, LinkedListNode<ProcessingNode>> ();
+			_processedMethods = new Dictionary<MethodDefinition, Instruction> ();
 		}
 
 		/// <summary>
@@ -120,52 +84,75 @@ void ProcessMethods (Collection<TypeDefinition> types)
 		/// It may optimize other methods as well - those are remembered for future reuse.
 		/// </summary>
 		/// <param name="method">The method to process</param>
-		void ProcessMethod (MethodDefinition method)
+		public void ProcessMethod (MethodDefinition method)
 		{
-			Debug.Assert (processingStack.Count == 0 && processingMethods.Count == 0);
-			processingStackVersion = 0;
+			if (!IsMethodSupported (method))
+				return;
+
+			if (_context.Annotations.GetAction (method.Module.Assembly) != AssemblyAction.Link)
+				return;
+
+			Debug.Assert (_processingStack.Count == 0 && _processingMethods.Count == 0);
+			_processingStackVersion = 0;
 
-			if (!processedMethods.ContainsKey (method)) {
+			if (!_processedMethods.ContainsKey (method)) {
 				AddMethodForProcessing (method);
 
 				ProcessStack ();
 			}
 
-			Debug.Assert (processedMethods.ContainsKey (method));
+			Debug.Assert (_processedMethods.ContainsKey (method));
+		}
+
+		static bool IsMethodSupported (MethodDefinition method)
+		{
+			if (!method.HasBody)
+				return false;
+
+			//
+			// Block methods which rewrite does not support
+			//
+			switch (method.ReturnType.MetadataType) {
+			case MetadataType.ByReference:
+			case MetadataType.FunctionPointer:
+				return false;
+			}
+
+			return true;
 		}
 
 		void AddMethodForProcessing (MethodDefinition method)
 		{
-			Debug.Assert (!processedMethods.ContainsKey (method));
+			Debug.Assert (!_processedMethods.ContainsKey (method));
 
 			var processingNode = new ProcessingNode (method, -1);
 
-			var stackNode = processingStack.AddFirst (processingNode);
-			processingMethods.Add (method, stackNode);
-			processingStackVersion++;
+			var stackNode = _processingStack.AddFirst (processingNode);
+			_processingMethods.Add (method, stackNode);
+			_processingStackVersion++;
 		}
 
 		void StoreMethodAsProcessedAndRemoveFromQueue (LinkedListNode<ProcessingNode> stackNode, Instruction methodValue)
 		{
-			Debug.Assert (stackNode.List == processingStack);
+			Debug.Assert (stackNode.List == _processingStack);
 			Debug.Assert (methodValue != null);
 
 			var method = stackNode.Value.Method;
-			processingMethods.Remove (method);
-			processingStack.Remove (stackNode);
-			processingStackVersion++;
+			_processingMethods.Remove (method);
+			_processingStack.Remove (stackNode);
+			_processingStackVersion++;
 
-			processedMethods[method] = methodValue;
+			_processedMethods[method] = methodValue;
 		}
 
 		void ProcessStack ()
 		{
-			while (processingStack.Count > 0) {
-				var stackNode = processingStack.First;
+			while (_processingStack.Count > 0) {
+				var stackNode = _processingStack.First;
 				var method = stackNode.Value.Method;
 
 				bool treatUnprocessedDependenciesAsNonConst = false;
-				if (stackNode.Value.LastAttemptStackVersion == processingStackVersion) {
+				if (stackNode.Value.LastAttemptStackVersion == _processingStackVersion) {
 					// Loop was detected - the stack hasn't changed since the last time we tried to process this method
 					// as such there's no way to resolve the situation (running the code below would produce the exact same result).
 
@@ -180,18 +167,18 @@ void ProcessStack ()
 					// To fix this go over the stack and find the "oldest" node with the current version - the "oldest" node which
 					// is part of the loop:
 					LinkedListNode<ProcessingNode> lastNodeWithCurrentVersion = null;
-					var candidateNodeToMoveToTop = processingStack.Last;
+					var candidateNodeToMoveToTop = _processingStack.Last;
 					bool foundNodesWithNonCurrentVersion = false;
 					while (candidateNodeToMoveToTop != stackNode) {
 						var previousNode = candidateNodeToMoveToTop.Previous;
 
-						if (candidateNodeToMoveToTop.Value.LastAttemptStackVersion == processingStackVersion) {
+						if (candidateNodeToMoveToTop.Value.LastAttemptStackVersion == _processingStackVersion) {
 							lastNodeWithCurrentVersion = candidateNodeToMoveToTop;
 						} else if (lastNodeWithCurrentVersion != null) {
 							// We've found the "oldest" node with current version and the current node is not of that version
 							// so it's older version. Move this node to the top of the stack.
-							processingStack.Remove (candidateNodeToMoveToTop);
-							processingStack.AddFirst (candidateNodeToMoveToTop);
+							_processingStack.Remove (candidateNodeToMoveToTop);
+							_processingStack.AddFirst (candidateNodeToMoveToTop);
 							foundNodesWithNonCurrentVersion = true;
 						}
 
@@ -215,14 +202,14 @@ void ProcessStack ()
 					treatUnprocessedDependenciesAsNonConst = true;
 				}
 
-				stackNode.Value = new ProcessingNode (stackNode.Value, processingStackVersion);
+				stackNode.Value = new ProcessingNode (stackNode.Value, _processingStackVersion);
 
-				if (!method.HasBody) {
+				if (!IsMethodSupported (method)) {
 					StoreMethodAsProcessedAndRemoveFromQueue (stackNode, ProcessedUnchangedSentinel);
 					continue;
 				}
 
-				var reducer = new BodyReducer (method.Body, Context);
+				var reducer = new BodyReducer (method.Body, _context);
 
 				//
 				// Temporary inlines any calls which return contant expression.
@@ -232,7 +219,7 @@ void ProcessStack ()
 				if (!TryInlineBodyDependencies (ref reducer, treatUnprocessedDependenciesAsNonConst, out bool changed)) {
 					// Method has unprocessed dependencies - so back off and try again later
 					// Leave it in the stack on its current position (it should not be on the first position anymore)
-					Debug.Assert (processingStack.First != stackNode);
+					Debug.Assert (_processingStack.First != stackNode);
 					continue;
 				}
 
@@ -251,7 +238,7 @@ void ProcessStack ()
 				// branch is replaced with nops.
 				//
 				if (reducer.RewriteBody ())
-					Context.LogMessage ($"Reduced '{reducer.InstructionsReplaced}' instructions in conditional branches for [{method.DeclaringType.Module.Assembly.Name}] method {method.GetDisplayName ()}");
+					_context.LogMessage ($"Reduced '{reducer.InstructionsReplaced}' instructions in conditional branches for [{method.DeclaringType.Module.Assembly.Name}] method {method.GetDisplayName ()}");
 
 				// Even if the rewriter doesn't find any branches to fold the inlining above may have changed the method enough
 				// such that we can now deduce its return value.
@@ -272,7 +259,7 @@ void ProcessStack ()
 					AnalyzeMethodForConstantResult (method, reducer.FoldedInstructions) ?? NonConstSentinel);
 			}
 
-			Debug.Assert (processingMethods.Count == 0);
+			Debug.Assert (_processingMethods.Count == 0);
 		}
 
 		Instruction AnalyzeMethodForConstantResult (MethodDefinition method, Collection<Instruction> instructions)
@@ -283,17 +270,17 @@ Instruction AnalyzeMethodForConstantResult (MethodDefinition method, Collection<
 			if (method.ReturnType.MetadataType == MetadataType.Void)
 				return null;
 
-			switch (Context.Annotations.GetAction (method)) {
+			switch (_context.Annotations.GetAction (method)) {
 			case MethodAction.ConvertToThrow:
 				return null;
 			case MethodAction.ConvertToStub:
-				return CodeRewriterStep.CreateConstantResultInstruction (Context, method);
+				return CodeRewriterStep.CreateConstantResultInstruction (_context, method);
 			}
 
 			if (method.IsIntrinsic () || method.NoInlining)
 				return null;
 
-			if (!Context.IsOptimizationEnabled (CodeOptimizations.IPConstantPropagation, method))
+			if (!_context.IsOptimizationEnabled (CodeOptimizations.IPConstantPropagation, method))
 				return null;
 
 			var analyzer = new ConstantExpressionMethodAnalyzer (method, instructions ?? method.Body.Instructions);
@@ -318,12 +305,12 @@ Instruction AnalyzeMethodForConstantResult (MethodDefinition method, Collection<
 		/// </returns>
 		bool TryGetConstantResultForMethod (MethodDefinition method, out Instruction constantResultInstruction)
 		{
-			if (!processedMethods.TryGetValue (method, out Instruction methodValue)) {
-				if (processingMethods.TryGetValue (method, out var stackNode)) {
+			if (!_processedMethods.TryGetValue (method, out Instruction methodValue)) {
+				if (_processingMethods.TryGetValue (method, out var stackNode)) {
 					// Method is already in the stack - not yet processed
 					// Move it to the top of the stack
-					processingStack.Remove (stackNode);
-					processingStack.AddFirst (stackNode);
+					_processingStack.Remove (stackNode);
+					_processingStack.AddFirst (stackNode);
 
 					// Note that stack version is not changing - we're just postponing work, not resolving anything.
 					// There's no result available for this method, so return false.
@@ -342,7 +329,7 @@ bool TryGetConstantResultForMethod (MethodDefinition method, out Instruction con
 				// Also its value has not been needed yet. Now we need to know if it's constant, so run the analyzer on it
 				var result = AnalyzeMethodForConstantResult (method, instructions: null);
 				Debug.Assert (result is Instruction || result == null);
-				processedMethods[method] = result ?? NonConstSentinel;
+				_processedMethods[method] = result ?? NonConstSentinel;
 				constantResultInstruction = result;
 			} else if (methodValue == NonConstSentinel) {
 				// Method was processed and found to not have a constant value
@@ -376,7 +363,7 @@ bool TryInlineBodyDependencies (ref BodyReducer reducer, bool treatUnprocessedDe
 					if (md.CallingConvention == MethodCallingConvention.VarArg)
 						break;
 
-					bool explicitlyAnnotated = Annotations.GetAction (md) == MethodAction.ConvertToStub;
+					bool explicitlyAnnotated = _context.Annotations.GetAction (md) == MethodAction.ConvertToStub;
 
 					// Allow inlining results of instance methods which are explicitly annotated
 					// but don't allow inling results of any other instance method.
@@ -432,7 +419,7 @@ bool TryInlineBodyDependencies (ref BodyReducer reducer, bool treatUnprocessedDe
 					if (field == null)
 						break;
 
-					if (Context.Annotations.TryGetFieldUserValue (field, out object value)) {
+					if (_context.Annotations.TryGetFieldUserValue (field, out object value)) {
 						targetResult = CodeRewriterStep.CreateConstantResultInstruction (field.FieldType, value);
 						if (targetResult == null)
 							break;

src/linker/Linker/Driver.cs

@@ -652,7 +652,6 @@ protected int SetupContext (ILogger customLogger = null)
 			if (removeCAS)
 				p.AddStepBefore (typeof (MarkStep), new RemoveSecurityStep ());
 
-			p.AddStepBefore (typeof (MarkStep), new RemoveUnreachableBlocksStep ());
 			p.AddStepBefore (typeof (MarkStep), new MarkExportedTypesTargetStep ());
 
 			p.AddStepBefore (typeof (OutputStep), new SealerStep ());