diff --git a/src/tools/illink/docs/design/constant-propagation-and-branch-removal.md b/src/tools/illink/docs/design/constant-propagation-and-branch-removal.md new file mode 100644 index 00000000000000..ee253a184e73c4 --- /dev/null +++ b/src/tools/illink/docs/design/constant-propagation-and-branch-removal.md @@ -0,0 +1,199 @@ +# Constant propagation and unreachable branch removal + +ILLink implements optimization which can propagate constant values across methods and based on these constants determine unreachable branches in code and remove those. This means that the code in the removed branch is not scanned for its dependencies which in turn won't be marked and can potentially be trimmed as well. + +## Desired behavior + +### Constant propagation + +Method can return constant value if its code will always return the same value (and it's possible to statically analyze that as a fact), for example: + +```csharp + public bool Is32Bit { get => false; } +``` + +On 64bit platforms the property is compiled with constant value, and ILLInk can determine this. It's also possible to use substitutions to overwrite method's return value to a constant via the [substitutions XML file](../data-formats.md#substitution-format). + +If such method is used in another method and it influences its return value, it can mean that the caller method will itself always return the same value. For example: + +```csharp + public int SizeOfIntPtr { + get { + if (Is32Bit) + return 4; + else + return 8; + } + } +``` + +ILLink will be able to determine that the call to `Is32Bit` getter will always return `false` and thus the `SizeOfIntPtr` will in turn always return `8`. + +### Unreachable branch removal + +If some method's return value is detected as constant, it can be possible to optimize conditions in which the return value is used and potentially even remove entire branches of code. For example: + +```csharp + public void CopyMemory () + { + if (Is32Bit) + { + CopyUsingDWords (); + } + else + { + CopyUsingQWords (); + } + } +``` + +In this case if building for 64bit platform the condition will be evaluated as `false` always, and thus the `true` branch of the `if` can be removed. This will in turn lead to also trimming `CopyUsingDWords` method (assuming it's not used from some other place). + +### Explicit non-goals + +For now ILLink will not inline any method calls. It's relatively tricky to determine if it's possible without breaking the application and leaving the actual calls in place makes debugging more predictable and easier (it's possible to set a breakpoint into the callee's body and it will be hit always). + +## Algorithm + +The implementation of this optimization is relatively complex since it's solving a potentially global problem in that results of optimization of one method potentially influence results of all methods which call it and so on. But we need the algorithm to work locally without global view. This is necessary because of lazy loading of assemblies, which means that before and during marking it's not guaranteed that all assemblies were discovered and loaded. At the same time this optimization must be complete before a given method is processed by `MarkStep` since we want to not mark dependencies from removed branches. + +### Used data structures + +* Dictionary of method -> value for all processed methods. The value of a method can be several things depending on the result of processing it: + * Sentinel value "Processed but not changed" which means the method has been processed and no optimization was done on it. It's unknown if the method returns a constant value or not (yet, analysis hasn't occurred). If nothing needs to know the return value of the method then this can be a final state. + * Sentinel value "Processed and is not constant" which means the method has been processed and its return value was not detected as constant. This is a final state. + * Instruction which represents the constant return value of the method if it was detected as returning constant value. This is a final state. + +* Processing stack which stores ordered list of processing node, each node representing a method and additional data about it. The stack is processed by always taking the top of the stack and attempting to process that node. Nodes are always added to the top of the stack and are always removed from the top of the stack. In some cases nodes are "moved", that is a node which is not on the top of the stack is moved to the top of the stack. For this reason the stack is implemented as a linked list (so that it's easy to point to nodes in it as well as moves nodes around). + +* Helper structure for method -> stack node fast lookups. + +### Processing methods + +It starts by placing the requested method on top of the stack and then processing the stack until it's empty (at which point the requested method is guaranteed to be processed). + +Processing the stack is a loop where: + +* Loop until stack is empty +* The top of the stack is peeked (not actually popped) and the method there is processed + 1. The last attempt version of the method is set to the current version of the stack (for loop detection, see below) + 2. The method's body is scanned and all callees which can be used for constant propagation are detected + * If the called method is already processed its value is used (if it has one) + * There's an optimization here where methods are only marked as processed without analyzing for their return value. If such method is encountered here, the return value analyzer will run in-place to determine the value of the method (and the result is stored) + * If the called method is not yet processed and is not on the stack, it's added to the top of the stack + * If the called method is not yet processed but it's already on the stack, it's moved to the top of the stack - this makes it efficient since this promotes processing of dependencies before the dependents and thus reduces the number of times the dependents must be re-scanned. + 3. If the scan was not fully done because some callees are not yet processed, give up on this method and loop (pick up the new top of the stack) + 4. If the scan was successful + * If there were not callees with constant values detected, mark the method as "Processed and unchanged" and remove it from the stack - loop + 5. If the method had any constants detected, run the branch removal logic to remove unused branches + 6. Regardless of branch removal results (even if nothing happened) use the new method body and the detected constants to analyze the method if it returns a constant itself - store the result + 7. Mark the method as processed and remove it from the stack - loop + +### Dependency loop detection + +The above algorithm could lead to endless loops if there's a recursion between multiple methods. For example code like this: + +```csharp +void A () { + if (Helper ()) + DoSomeWork (); + + return B (); +} + +void B () { + if (Helper ()) + DoSomeWork (); + + return A (); +} +``` + +In this case when `A`'s body is scanned (step 2 above) it will find a call to `B` and add it for processing and back off. Then `B` is top of the stack so it's scanned, and finds `A` to be on the stack but not yet processed. So it moves it to the top of the stack and backs off. Then `A` is processed... and so on. + +To avoid this a versioning scheme is used to detect loops. There's a global version number maintained alongside the stack. Every time a new item is added or removed from the stack the stack version is incremented. This is used to detect "something has changed". Each node on the stack stores the stack version from the last time it was attempted to process that node/method. So in the above sample the flow would be something like: + +* Stack `StackVersion = 0` +* `A` is added to the stack - `StackVersion = 1` +* `A` is attempted to be processed - `A.LastAttemptVersion = 1` +* `A` detects dependency on `B` and adds `B` to the top of the stack - `StackVersion = 2` +* `B` is attempted to be processed - `B.LastAttemptVersion = 2` +* `B` detects dependency on `A` and moves `A` to the top of the stack - no version changes - still 2 +* `A` is attempted to be processed - `A.LastAttemptVersion = 2` +* `A` detects dependency on `B` and moves `B` to the top of the stack - no version changes - still 2 +* `B` is attempted to be processed - at this point `B.LastAttemptVersion == 2` and also `StackVersion == 2` + +To detect the loop each time a node is about to be processed its `LastAttemptVersion` is checked against the current stack version. If they're equal it means that nothing changes since last time the node was attempted to be processed. So it's expected that processing it again would produce the same results (that is no results, still has unprocessed dependencies). That's the cases where loop is detected. + +### Dependency loop resolution + +Once the loop is detected the algorithm has to make some changes to avoid looping forever. This is done by force processing one of the methods in the loop and thus removing it from the stack. To do this: + +* The method at the top of stack at point of loop detections is processed +* When scanning its dependencies (step 2) it treats all unprocessed dependencies as "processed with non-constant result" +* This means the scan will always succeed (there won't be any unprocessed dependencies blocking the scan from finishing successfully) +* At this point the method is processed as normal - starting with step 4 above. This means the method will be marked as processed with some result and will be removed from the stack +* Since it will be removed from the stack, the stack version will be incremented - this means that the other methods on the stack will not detect loop again and will try to process again + +Since this resolves one of the method in the loop, it should break the loop and he algorithm should be able to move on. If that's not the case, the loop detection will kick in with some higher version again and another method will be force-processed and so on. + +### Complex loop cases + +The above could still lead to undesirable behavior. In the sample above both `A` and `B` have a dependency on `Helper`. So far this was ignored in the description, but given the algorithm above, `Helper` will not be processed before the loop is detected. It will be added onto the stack, but at the end of scanning either `A` or `B` it will never become the top of the stack (since `A` ends with added `B` to the top, and `B` ends with adding `A` to the top). So it will never be even attempted to be processed. So if `Helper` is constant `false` the method which we force-process (`B` in the above example) will not see it that way and will treat it as non-const. This leads to not removing the call to `DoSomeWork`. This is wrong in the sense that the optimization should remove this call. Especially if `Helper` is driven by a [feature switch](https://github.com/dotnet/designs/blob/main/accepted/2020/feature-switch.md) such behavior is highly undesirable (it's not only a size issue where it keeps more code than necessary, but it could mean generating warnings from the code which should be removed). + +To mitigate this the algorithm will do one more step before breaking the loop. If the loop is detected it will go over the stack from top to bottom and it will look for the last node with the current version (so basically the last node which is part of the loop, since all nodes which are part of the loop will end up with the same current version). Once it finds that node (there should be at least one more except the one on top of the stack) it will go over all the nodes from that node to the top of the stack and if it finds a node which doesn't have a current version (meaning it's not part of the loop and was not attempted to be processed recently) it will move it to the top of the stack - no version change. + +If any such node is found, normal processing will resume. In the above example this will mean that `Helper` gets to the top of the stack, will be processed and removed from the stack (stack version increments). Now `A` and `B` will be processed again, eventually detecting the loop again. At this point the search for nodes which are not current version will end up empty. Only at that point the loop will be broken by force-processing one of the methods in the loop. + +For illustration the flow with `Helper` being considered. A `Main` is added to put another method which illustrates the algorithm better: + +* Stack `StackVersion = 0` +* `Main` is added to the stack - `StackVersion = 1` +* `Main` is attempted to be processed - `Main.LastAttemptVersion = 1` +* `A` is added to the stack - `StackVersion = 2` +* `A` is attempted to be processed - `A.LastAttemptVersion = 2` +* `A` detects dependency on `Helper` and adds `Helper` to the top of the stack - `StackVersion = 3` +* `A` detects dependency on `B` and adds `B` to the top of the stack - `StackVersion = 4` +* `B` is attempted to be processed - `B.LastAttemptVersion = 4` +* `B` detects dependency on `Helper` and moves `Helper` to the top of the stack - no version change - still 4 +* `B` detects dependency on `A` and moves `A` to the top of the stack - no version changes - still 4 +* `A` is attempted to be processed - `A.LastAttemptVersion = 4` +* `A` detects dependency on `Helper` and moves `Helper` to the top of the stack - no version change - still 4 +* `A` detects dependency on `B` and moves `B` to the top of the stack - no version changes - still 4 +* `B` is attempted to be processed - at this point `B.LastAttemptVersion == 4` and also `StackVersion == 4` + +At this point the stack looks like this (top is the first line below) - `StackVersion == 4`: + +* `B` - `LastAttemptVersion = 4` +* `Helper` - `LastAttemptVersion = -1` (never attempted to be processed) +* `A` - `LastAttemptVersion = 4` +* `Main` - `LastAttemptVersion = 1` + +The algorithm above will go over the stack to find the "oldest" node with version `4` (the current version) - and will find `A`. Then it will go from `A` back to the top searching for nodes with version `!= 4`, it will find `Helper`. It moves `Helper` to the top of the stack. Then normal processing resumes + +* `Helper` is processed fully (no dependencies) - removed from the stack - `StackVersion = 5` +* `B` is attempted to be processed - `B.LastAttemptVersion = 5` +* `B` detects dependency on `A` and moves `A` to the top of the stack - no version changes - still 5 +* `A` is attempted to be processed - `A.LastAttemptVersion = 5` +* `A` detects dependency on `B` and moves `B` to the top of the stack - no version changes - still 5 +* `B` is attempted to be processed - at this point `B.LastAttemptVersion == 5` and also `StackVersion == 5` + +A loop is detected again - `StackVersion == 5` - the stack looks like this: + +* `B` - `LastAttemptVersion = 5` +* `A` - `LastAttemptVersion = 5` +* `Main` - `LastAttemptVersion = 1` + +The scan over the stack won't find any methods to move to the top. So it will break the loop by force-processing `B` and considering its dependency on `A` as non-const. `B` is removed from the stack - `StackVersion = 6`. Processing removes and now `A` will process fully since all of its dependencies are resolved and so on... + +## Alternatives and improvements + +### Use actual recursion in the analyzer + +The processing of methods is recursive in nature since callers needs to know results of processing callees. To avoid actual recursion in the analyzer, the nodes are stored in the processing stack. If the necessary results are not yet known for a given method, the current method is postponed (moves down on the stack) and it will be retried later on. This is potentially expensive. An optimization would be to allow a limited recursion within the analyzer and only rely on the processing stack in cases a recursion limit is reached. + +### Avoid scanning of potentially removed branches + +Currently the scanning (step 2 above) goes over all instructions in the method's body and will request processing of all called methods. This means that even if the called method is behind a feature check which is disabled, it will still be processed (and all of its dependencies will be processed as well). In the end that branch will be removed and none of those methods will end up being marked. All the processing of those methods will be effectively thrown away. + +To improve this behavior we would need to merge the scanning with the constant condition detection and branch removal. Basically steps 2-5 would have to become one. The idea is that the scanning would only request processing for methods which are on the main path through the method or in branches which can't be removed. This would probably mean that scanning would have to give up sooner (currently it always goes over the whole method body requesting processing of all dependencies) which wold likely lead to more frequent re-scanning of the method (to eventually reach the end). The advantage would be the potential to not process methods which are not needed. An experiment would have to be done to measure the numbers to determine if this is actually a beneficial change. diff --git a/src/tools/illink/src/linker/Linker.Steps/RemoveUnreachableBlocksStep.cs b/src/tools/illink/src/linker/Linker.Steps/RemoveUnreachableBlocksStep.cs index 859d65aaeca400..5ccb40e4409341 100644 --- a/src/tools/illink/src/linker/Linker.Steps/RemoveUnreachableBlocksStep.cs +++ b/src/tools/illink/src/linker/Linker.Steps/RemoveUnreachableBlocksStep.cs @@ -16,43 +16,266 @@ namespace Mono.Linker.Steps // public class RemoveUnreachableBlocksStep : BaseStep { - Dictionary constExprMethods; - bool constExprMethodsAdded; MethodDefinition IntPtrSize, UIntPtrSize; + readonly struct ProcessingNode + { + public ProcessingNode (MethodDefinition method, int lastAttemptStackVersion) + { + Method = method; + LastAttemptStackVersion = lastAttemptStackVersion; + } + + public ProcessingNode (in ProcessingNode other, int newLastAttempStackVersion) + { + Method = other.Method; + LastAttemptStackVersion = newLastAttempStackVersion; + } + + public readonly MethodDefinition Method; + public readonly int LastAttemptStackVersion; + } + + // 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 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. + // This is used to track loops - if there are two methods which have dependencies on each other + // the processing needs to detect that and mark at least one of them as nonconst (regardless of the method's body) + // to break the loop. + // This is done by storing the version of the stack on each method node when that method is processed, + // 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; + + // 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> 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. + // The value in this dictionary can be + // - ProcessedUnchangedSentinel - method has been processed and nothing was changed on it - its value is unknown + // - NonConstSentinel - method has been processed and the return value is not a const + // - 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 processedMethods; + static readonly Instruction ProcessedUnchangedSentinel = Instruction.Create (OpCodes.Ldstr, "ProcessedUnchangedSentinel"); + static readonly Instruction NonConstSentinel = Instruction.Create (OpCodes.Ldstr, "NonConstSentinel"); + protected override void Process () { var assemblies = Context.Annotations.GetAssemblies ().ToArray (); - constExprMethods = new Dictionary (); + processingStack = new LinkedList (); + processingMethods = new Dictionary> (); + processedMethods = new Dictionary (); - do { - // - // Body rewriting can produce more methods with constant expression - // - constExprMethodsAdded = false; + foreach (var assembly in assemblies) { + if (Annotations.GetAction (assembly) != AssemblyAction.Link) + continue; + + ProcessMethods (assembly.MainModule.Types); + } + } + + void ProcessMethods (Collection types) + { + foreach (var type in types) { + if (type.IsInterface) + continue; - foreach (var assembly in assemblies) { - if (Annotations.GetAction (assembly) != AssemblyAction.Link) + 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; + } - RewriteBodies (assembly.MainModule.Types); + ProcessMethod (method); } - } while (constExprMethodsAdded); + + if (type.HasNestedTypes) + ProcessMethods (type.NestedTypes); + } } - bool TryGetConstantResultInstructionForMethod (MethodDefinition method, out Instruction constantResultInstruction) + /// + /// Processes the specified and method and perform all branch removal optimizations on it. + /// When this returns it's guaranteed that the method has been optimized (if possible). + /// It may optimize other methods as well - those are remembered for future reuse. + /// + /// The method to process + void ProcessMethod (MethodDefinition method) { - if (constExprMethods.TryGetValue (method, out constantResultInstruction)) - return constantResultInstruction != null; + Debug.Assert (processingStack.Count == 0 && processingMethods.Count == 0); + processingStackVersion = 0; - constantResultInstruction = GetConstantResultInstructionForMethod (method, instructions: null); - constExprMethods.Add (method, constantResultInstruction); + if (!processedMethods.ContainsKey (method)) { + AddMethodForProcessing (method); - return constantResultInstruction != null; + ProcessStack (); + } + + Debug.Assert (processedMethods.ContainsKey (method)); } - Instruction GetConstantResultInstructionForMethod (MethodDefinition method, Collection instructions) + void AddMethodForProcessing (MethodDefinition method) + { + Debug.Assert (!processedMethods.ContainsKey (method)); + + var processingNode = new ProcessingNode (method, -1); + + var stackNode = processingStack.AddFirst (processingNode); + processingMethods.Add (method, stackNode); + processingStackVersion++; + } + + void StoreMethodAsProcessedAndRemoveFromQueue (LinkedListNode stackNode, Instruction methodValue) + { + Debug.Assert (stackNode.List == processingStack); + Debug.Assert (methodValue != null); + + var method = stackNode.Value.Method; + processingMethods.Remove (method); + processingStack.Remove (stackNode); + processingStackVersion++; + + processedMethods[method] = methodValue; + } + + void ProcessStack () + { + while (processingStack.Count > 0) { + var stackNode = processingStack.First; + var method = stackNode.Value.Method; + + bool treatUnprocessedDependenciesAsNonConst = false; + 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). + + // Observation: + // All nodes on the stack which have `LastAttemptStackVersion` equal to `processingStackVersion` are part of the loop + // meaning removing any of them should break the loop and allow to make progress. + // There might be other methods in between these which don't have the current version but are dependencies of some of the method + // in the loop. + // If we don't process these, then we might miss constants and branches to remove. See the doc + // `constant-propagation-and-branch-removal.md` in this repo for more details and a sample. + + // 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 lastNodeWithCurrentVersion = null; + var candidateNodeToMoveToTop = processingStack.Last; + bool foundNodesWithNonCurrentVersion = false; + while (candidateNodeToMoveToTop != stackNode) { + var previousNode = candidateNodeToMoveToTop.Previous; + + 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); + foundNodesWithNonCurrentVersion = true; + } + + candidateNodeToMoveToTop = previousNode; + } + + // There should be at least 2 nodes with the latest version to form a loop + Debug.Assert (lastNodeWithCurrentVersion != stackNode); + + // If any node was found which was not of current version (and moved to the top of the stack), move on to processing + // the stack - this will give a chance for these methods to be processed. It doesn't break the loop and we should come back here + // again due to the same loop as before, but now with more nodes processed (hopefully all of the dependencies of the nodes in the loop). + // In the worst case all of those nodes will become part of the loop - in which case we will move on to break the loop anyway. + if (foundNodesWithNonCurrentVersion) { + continue; + } + + // No such node was found -> we only have nodes in the loop now, so we have to break the loop. + // We do this by processing it with special flag which will make it ignore any unprocessed dependencies + // treating them as non-const. These should only be nodes in the loop. + treatUnprocessedDependenciesAsNonConst = true; + } + + stackNode.Value = new ProcessingNode (stackNode.Value, processingStackVersion); + + if (!method.HasBody) { + StoreMethodAsProcessedAndRemoveFromQueue (stackNode, ProcessedUnchangedSentinel); + continue; + } + + var reducer = new BodyReducer (method.Body, Context); + + // + // Temporary inlines any calls which return contant expression. + // If it needs to know the result of analysis of other methods and those has not been processed yet + // it will still scan the entire body, but we will return the full processing one more time. + // + 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); + continue; + } + + if (!changed) { + // All dependencies are processed and there were no const values found. There's nothing to optimize. + // Mark the method as processed - without computing the const value of it (we don't know if it's going to be needed) + StoreMethodAsProcessedAndRemoveFromQueue (stackNode, ProcessedUnchangedSentinel); + continue; + } + + // The method has been modified due to constant propagation - we will optimize it. + + // + // This is the main step which evaluates if inlined calls can + // produce folded branches. When it finds them the unreachable + // 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 ()}"); + + // 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. + + if (method.ReturnType.MetadataType == MetadataType.Void) { + // Method is fully processed and can't be const (since it doesn't return value) - so mark it as processed without const value + StoreMethodAsProcessedAndRemoveFromQueue (stackNode, NonConstSentinel); + continue; + } + + // + // Run the analyzer in case body change rewrote it to constant expression + // Note that we have to run it always (even if we may not need the result ever) since it depends on the temporary inlining above + // Otherwise we would have to remember the inlined code along with the method. + // + StoreMethodAsProcessedAndRemoveFromQueue ( + stackNode, + AnalyzeMethodForConstantResult (method, reducer.FoldedInstructions) ?? NonConstSentinel); + } + + Debug.Assert (processingMethods.Count == 0); + } + + Instruction AnalyzeMethodForConstantResult (MethodDefinition method, Collection instructions) { if (!method.HasBody) return null; @@ -81,74 +304,61 @@ Instruction GetConstantResultInstructionForMethod (MethodDefinition method, Coll return null; } - void RewriteBodies (Collection types) + /// + /// Determines if a method has constant return value. If the method has not yet been processed it makes sure + /// it is on the stack for processing and returns without a result. + /// + /// The method to determine result for + /// If successfull and the method returns a constant value this will be set to the + /// instruction with the constant value. If successfulll and the method doesn't have a constant value this is set to null. + /// + /// true - if the method was analyzed and result is known + /// constantResultInstruction is set to an instance if the method returns a constant, otherwise it's set to null + /// false - if the method has not yet been analyzed and the caller should retry later + /// + bool TryGetConstantResultForMethod (MethodDefinition method, out Instruction constantResultInstruction) { - 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; - } - RewriteBody (method); + 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); + + // 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. + constantResultInstruction = null; + return false; } - if (type.HasNestedTypes) - RewriteBodies (type.NestedTypes); + // Method is not yet in the stack - add it there + AddMethodForProcessing (method); + constantResultInstruction = null; + return false; } - } - - void RewriteBody (MethodDefinition method) - { - var reducer = new BodyReducer (method.Body, Context); - - // - // Temporary inlines any calls which return contant expression - // - if (!TryInlineBodyDependencies (ref reducer)) - return; - - // - // This is the main step which evaluates if inlined calls can - // produce folded branches. When it finds them the unreachable - // 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 ()}"); - // 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. - - if (method.ReturnType.MetadataType == MetadataType.Void) - return; - - if (!constExprMethods.TryGetValue (method, out var constInstruction) || constInstruction == null) { - // - // Re-run the analyzer in case body change rewrote it to constant expression - // - var constantResultInstruction = GetConstantResultInstructionForMethod (method, reducer.FoldedInstructions); - if (constantResultInstruction != null) { - constExprMethods[method] = constantResultInstruction; - constExprMethodsAdded = true; - } + if (methodValue == ProcessedUnchangedSentinel) { + // Method has been processed and no changes has been made to it. + // 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; + constantResultInstruction = result; + } else if (methodValue == NonConstSentinel) { + // Method was processed and found to not have a constant value + constantResultInstruction = null; + } else { + // Method was already processed and found to have a constant value + constantResultInstruction = methodValue; } + + return true; } - bool TryInlineBodyDependencies (ref BodyReducer reducer) + bool TryInlineBodyDependencies (ref BodyReducer reducer, bool treatUnprocessedDependenciesAsNonConst, out bool changed) { - bool changed = false; + changed = false; + bool hasUnprocessedDependencies = false; var instructions = reducer.Body.Instructions; Instruction targetResult; @@ -194,8 +404,22 @@ bool TryInlineBodyDependencies (ref BodyReducer reducer) break; } - if (!TryGetConstantResultInstructionForMethod (md, out targetResult)) + if (md == reducer.Body.Method) { + // Special case for direct recursion - simply assume non-const value + // since we can't tell. + break; + } + + if (!TryGetConstantResultForMethod (md, out targetResult)) { + if (!treatUnprocessedDependenciesAsNonConst) + hasUnprocessedDependencies = true; break; + } else if (targetResult == null || hasUnprocessedDependencies) { + // Even is const is detected, there's no point in rewriting anything + // if we've found unprocessed dependency since the results of this scan will + // be thrown away (we back off and wait for the unprocessed dependency to be processed first). + break; + } reducer.Rewrite (i, targetResult); changed = true; @@ -234,7 +458,15 @@ bool TryInlineBodyDependencies (ref BodyReducer reducer) sizeOfImpl = (IntPtrSize ??= FindSizeMethod (operand.Resolve ())); } - if (sizeOfImpl != null && TryGetConstantResultInstructionForMethod (sizeOfImpl, out targetResult)) { + if (sizeOfImpl != null) { + if (!TryGetConstantResultForMethod (sizeOfImpl, out targetResult)) { + if (!treatUnprocessedDependenciesAsNonConst) + hasUnprocessedDependencies = true; + break; + } else if (targetResult == null || hasUnprocessedDependencies) { + break; + } + reducer.Rewrite (i, targetResult); changed = true; } @@ -243,7 +475,7 @@ bool TryInlineBodyDependencies (ref BodyReducer reducer) } } - return changed; + return !hasUnprocessedDependencies; } static MethodDefinition FindSizeMethod (TypeDefinition type) diff --git a/src/tools/illink/test/Mono.Linker.Tests.Cases/UnreachableBlock/BodiesWithSubstitutions.cs b/src/tools/illink/test/Mono.Linker.Tests.Cases/UnreachableBlock/BodiesWithSubstitutions.cs index 1a0773576198b7..f1ebbdc14b26e7 100644 --- a/src/tools/illink/test/Mono.Linker.Tests.Cases/UnreachableBlock/BodiesWithSubstitutions.cs +++ b/src/tools/illink/test/Mono.Linker.Tests.Cases/UnreachableBlock/BodiesWithSubstitutions.cs @@ -37,6 +37,8 @@ public static void Main () LoopWithoutConstants.Test (); LoopWithConstants.Test (); + LoopWithConstantsComplex.Test (); + MultiLoopWithConstantsComplex.Test (); DeepConstant.Test (); ConstantFromNewAssembly.Test (); @@ -190,6 +192,136 @@ public static void Test () } } + static class LoopWithConstantsComplex + { + [Kept] + static int depth = 0; + + [Kept] + static bool IsTrue () + { + return true; + } + + static void ShouldNotBeReached () { } + + [Kept] + [ExpectBodyModified] + static bool LoopMethod1 () + { + depth++; + if (!IsTrue ()) + ShouldNotBeReached (); + + return LoopMethod2 (); + } + + [Kept] + [ExpectBodyModified] + static bool LoopMethod2 () + { + if (!IsTrue ()) + ShouldNotBeReached (); + + if (depth < 100) + LoopMethod1 (); + + return false; + } + + [Kept] static void Reached () { } + [Kept] static void Reached2 () { } + + [Kept] + public static void Test () + { + // Currently we don't recognize this pattern as constant + // Technically LoopMethod1 will always return false + if (LoopMethod1 ()) + Reached (); + else + Reached2 (); + } + } + + static class MultiLoopWithConstantsComplex + { + [Kept] + static int depth = 0; + + [Kept] + static bool IsTrue () + { + return true; + } + + static void ShouldNotBeReached () { } + + [Kept] + [ExpectBodyModified] + static bool InnerLoopMethod1 () + { + depth++; + if (!IsTrue ()) + ShouldNotBeReached (); + + return InnerLoopMethod2 (); + } + + [Kept] + [ExpectBodyModified] + static bool InnerLoopMethod2 () + { + if (!IsTrue ()) + ShouldNotBeReached (); + + if (depth < 100) + InnerLoopMethod1 (); + + return false; + } + + [Kept] + static void InnerReached () { } + + [Kept] + [ExpectBodyModified] + static bool OuterLoopMethod1 () + { + if (!IsTrue ()) + ShouldNotBeReached (); + + // Currently we don't recognize this pattern as constant + if (InnerLoopMethod1 ()) + InnerReached (); + + return OuterLoopMethod2 (); + } + + [Kept] + [ExpectBodyModified] + static bool OuterLoopMethod2 () + { + if (!IsTrue ()) + ShouldNotBeReached (); + + return OuterLoopMethod1 (); + } + + [Kept] static void Reached () { } + [Kept] static void Reached2 () { } + + [Kept] + public static void Test () + { + // Currently we don't recognize this pattern as constant + if (OuterLoopMethod1 ()) + Reached (); + else + Reached2 (); + } + } + static class DeepConstant { [Kept] static bool Method1 () => Method2 ();