Extract shared IL pattern analysis to a class (#103701)

This fixes the problem discussed at #102248 (comment). Now we call into the same code from both substitutions and scanner.

Author: MichalStrehovsky

src/coreclr/nativeaot/Test.CoreLib/src/System/RuntimeTypeHandle.cs

@@ -34,5 +34,10 @@ private static RuntimeTypeHandle GetRuntimeTypeHandle(IntPtr pEEType)
         {
             return new RuntimeTypeHandle(pEEType);
         }
+
+        private static Type GetRuntimeType(IntPtr pEEType)
+        {
+            return Type.GetTypeFromHandle(new RuntimeTypeHandle(pEEType));
+        }
     }
 }

src/coreclr/tools/Common/TypeSystem/IL/ILImporter.cs

@@ -114,8 +114,9 @@ private void FindJumpTargets()
                 MarkInstructionBoundary();
 
                 ILOpcode opCode = (ILOpcode)ReadILByte();
+                if (opCode == ILOpcode.prefix1)
+                    opCode = (ILOpcode)(0x100 + ReadILByte());
 
-            again:
                 switch (opCode)
                 {
                     case ILOpcode.ldarg_s:
@@ -179,9 +180,6 @@ private void FindJumpTargets()
                     case ILOpcode.sizeof_:
                         SkipIL(4);
                         break;
-                    case ILOpcode.prefix1:
-                        opCode = (ILOpcode)(0x100 + ReadILByte());
-                        goto again;
                     case ILOpcode.br_s:
                     case ILOpcode.leave_s:
                         {
@@ -314,6 +312,8 @@ private void MarkBasicBlock(BasicBlock basicBlock)
             }
         }
 
+        partial void StartImportingInstruction(ILOpcode opcode);
+
         private void ImportBasicBlock(BasicBlock basicBlock)
         {
             _currentBasicBlock = basicBlock;
@@ -324,8 +324,11 @@ private void ImportBasicBlock(BasicBlock basicBlock)
                 StartImportingInstruction();
 
                 ILOpcode opCode = (ILOpcode)ReadILByte();
+                if (opCode == ILOpcode.prefix1)
+                    opCode = (ILOpcode)(0x100 + ReadILByte());
+
+                StartImportingInstruction(opCode);
 
-            again:
                 switch (opCode)
                 {
                     case ILOpcode.nop:
@@ -814,9 +817,6 @@ private void ImportBasicBlock(BasicBlock basicBlock)
                     case ILOpcode.conv_u:
                         ImportConvert(WellKnownType.UIntPtr, false, true);
                         break;
-                    case ILOpcode.prefix1:
-                        opCode = (ILOpcode)(0x100 + ReadILByte());
-                        goto again;
                     case ILOpcode.arglist:
                         ImportArgList();
                         break;

src/coreclr/tools/Common/TypeSystem/IL/ILReader.cs

@@ -100,7 +100,7 @@ public ILOpcode ReadILOpcode()
             return opcode;
         }
 
-        public ILOpcode PeekILOpcode()
+        public readonly ILOpcode PeekILOpcode()
         {
             ILOpcode opcode = (ILOpcode)_ilBytes[_currentOffset];
             if (opcode == ILOpcode.prefix1)
@@ -113,6 +113,14 @@ public ILOpcode PeekILOpcode()
             return opcode;
         }
 
+        public readonly int PeekILToken()
+        {
+            if (!BinaryPrimitives.TryReadInt32LittleEndian(_ilBytes.Slice(_currentOffset), out int value))
+                ThrowHelper.ThrowInvalidProgramException();
+
+            return value;
+        }
+
         public void Skip(ILOpcode opcode)
         {
             if (!opcode.IsValid())

src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/SubstitutedILProvider.cs

@@ -252,13 +252,17 @@ public MethodIL GetMethodILWithInlinedSubstitutions(MethodIL method)
                 if ((flags[offset] & OpcodeFlags.Mark) != 0)
                     continue;
 
+                TypeEqualityPatternAnalyzer typeEqualityAnalyzer = default;
+
                 ILReader reader = new ILReader(methodBytes, offset);
                 while (reader.HasNext)
                 {
                     offset = reader.Offset;
                     flags[offset] |= OpcodeFlags.Mark;
                     ILOpcode opcode = reader.ReadILOpcode();
 
+                    typeEqualityAnalyzer.Advance(opcode, reader, method);
+
                     // Mark any applicable EH blocks
                     foreach (ILExceptionRegion ehRegion in ehRegions)
                     {
@@ -297,7 +301,8 @@ public MethodIL GetMethodILWithInlinedSubstitutions(MethodIL method)
                         || opcode == ILOpcode.brtrue || opcode == ILOpcode.brtrue_s)
                     {
                         int destination = reader.ReadBranchDestination(opcode);
-                        if (!TryGetConstantArgument(method, methodBytes, flags, offset, 0, out int constant))
+                        if (!TryGetConstantArgument(method, methodBytes, flags, offset, 0, out int constant)
+                            && !TryExpandTypeEquality(typeEqualityAnalyzer, method, out constant))
                         {
                             // Can't get the constant - both branches are live.
                             offsetsToVisit.Push(destination);
@@ -681,13 +686,6 @@ private bool TryGetConstantArgument(MethodIL methodIL, byte[] body, OpcodeFlags[
                             constant = (int)substitution.Value;
                             return true;
                         }
-                        else if (method.IsIntrinsic && method.Name is "op_Inequality" or "op_Equality"
-                            && method.OwningType is MetadataType mdType
-                            && mdType.Name == "Type" && mdType.Namespace == "System" && mdType.Module == mdType.Context.SystemModule
-                            && TryExpandTypeEquality(methodIL, body, flags, currentOffset, method.Name, out constant))
-                        {
-                            return true;
-                        }
                         else if (method.IsIntrinsic && method.Name is "get_IsValueType" or "get_IsEnum"
                             && method.OwningType is MetadataType mdt
                             && mdt.Name == "Type" && mdt.Namespace == "System" && mdt.Module == mdt.Context.SystemModule
@@ -873,175 +871,63 @@ private static bool TryExpandTypeIs(MethodIL methodIL, byte[] body, OpcodeFlags[
             return true;
         }
 
-        private bool TryExpandTypeEquality(MethodIL methodIL, byte[] body, OpcodeFlags[] flags, int offset, string op, out int constant)
-        {
-            if (TryExpandTypeEquality_TokenToken(methodIL, body, flags, offset, out constant)
-                || TryExpandTypeEquality_TokenOther(methodIL, body, flags, offset, 1, expectGetType: false, out constant)
-                || TryExpandTypeEquality_TokenOther(methodIL, body, flags, offset, 2, expectGetType: false, out constant)
-                || TryExpandTypeEquality_TokenOther(methodIL, body, flags, offset, 3, expectGetType: false, out constant)
-                || TryExpandTypeEquality_TokenOther(methodIL, body, flags, offset, 1, expectGetType: true, out constant)
-                || TryExpandTypeEquality_TokenOther(methodIL, body, flags, offset, 2, expectGetType: true, out constant)
-                || TryExpandTypeEquality_TokenOther(methodIL, body, flags, offset, 3, expectGetType: true, out constant))
-            {
-                if (op == "op_Inequality")
-                    constant ^= 1;
-
-                return true;
-            }
-
-            return false;
-        }
-
-        private static bool TryExpandTypeEquality_TokenToken(MethodIL methodIL, byte[] body, OpcodeFlags[] flags, int offset, out int constant)
+        private bool TryExpandTypeEquality(in TypeEqualityPatternAnalyzer analyzer, MethodIL methodIL, out int constant)
         {
-            // We expect to see a sequence:
-            // ldtoken Foo
-            // call GetTypeFromHandle
-            // ldtoken Bar
-            // call GetTypeFromHandle
-            // -> offset points here
             constant = 0;
-            const int SequenceLength = 20;
-            if (offset < SequenceLength)
-                return false;
-
-            if ((flags[offset - SequenceLength] & OpcodeFlags.InstructionStart) == 0)
+            if (!analyzer.IsTypeEqualityBranch)
                 return false;
 
-            ILReader reader = new ILReader(body, offset - SequenceLength);
-
-            TypeDesc type1 = ReadLdToken(ref reader, methodIL, flags);
-            if (type1 == null)
-                return false;
-
-            if (!ReadGetTypeFromHandle(ref reader, methodIL, flags))
-                return false;
-
-            TypeDesc type2 = ReadLdToken(ref reader, methodIL, flags);
-            if (type2 == null)
-                return false;
-
-            if (!ReadGetTypeFromHandle(ref reader, methodIL, flags))
-                return false;
-
-            // No value in making this work for definitions
-            if (type1.IsGenericDefinition || type2.IsGenericDefinition)
-                return false;
-
-            // Dataflow runs on top of uninstantiated IL and we can't answer some questions there.
-            // Unfortunately this means dataflow will still see code that the rest of the system
-            // might have optimized away. It should not be a problem in practice.
-            if (type1.ContainsSignatureVariables() || type2.ContainsSignatureVariables())
-                return false;
-
-            bool? equality = TypeExtensions.CompareTypesForEquality(type1, type2);
-            if (!equality.HasValue)
-                return false;
-
-            constant = equality.Value ? 1 : 0;
-
-            return true;
-        }
-
-        private bool TryExpandTypeEquality_TokenOther(MethodIL methodIL, byte[] body, OpcodeFlags[] flags, int offset, int ldInstructionSize, bool expectGetType, out int constant)
-        {
-            // We expect to see a sequence:
-            // ldtoken Foo
-            // call GetTypeFromHandle
-            // ldloc.X/ldloc_s X/ldarg.X/ldarg_s X
-            // [optional] call Object.GetType
-            // -> offset points here
-            //
-            // The ldtoken part can potentially be in the second argument position
-
-            constant = 0;
-            int sequenceLength = 5 + 5 + ldInstructionSize + (expectGetType ? 5 : 0);
-            if (offset < sequenceLength)
-                return false;
-
-            if ((flags[offset - sequenceLength] & OpcodeFlags.InstructionStart) == 0)
-                return false;
-
-            ILReader reader = new ILReader(body, offset - sequenceLength);
+            if (analyzer.IsTwoTokens)
+            {
+                var type1 = (TypeDesc)methodIL.GetObject(analyzer.Token1);
+                var type2 = (TypeDesc)methodIL.GetObject(analyzer.Token2);
 
-            TypeDesc knownType = null;
+                // No value in making this work for definitions
+                if (type1.IsGenericDefinition || type2.IsGenericDefinition)
+                    return false;
 
-            // Is the ldtoken in the first position?
-            if (reader.PeekILOpcode() == ILOpcode.ldtoken)
-            {
-                knownType = ReadLdToken(ref reader, methodIL, flags);
-                if (knownType == null)
+                // Dataflow runs on top of uninstantiated IL and we can't answer some questions there.
+                // Unfortunately this means dataflow will still see code that the rest of the system
+                // might have optimized away. It should not be a problem in practice.
+                if (type1.ContainsSignatureVariables() || type2.ContainsSignatureVariables())
                     return false;
 
-                if (!ReadGetTypeFromHandle(ref reader, methodIL, flags))
+                bool? equality = TypeExtensions.CompareTypesForEquality(type1, type2);
+                if (!equality.HasValue)
                     return false;
-            }
 
-            ILOpcode opcode = reader.ReadILOpcode();
-            if (ldInstructionSize == 1 && opcode is (>= ILOpcode.ldloc_0 and <= ILOpcode.ldloc_3) or (>= ILOpcode.ldarg_0 and <= ILOpcode.ldarg_3))
-            {
-                // Nothing to read
-            }
-            else if (ldInstructionSize == 2 && opcode is ILOpcode.ldloc_s or ILOpcode.ldarg_s)
-            {
-                reader.ReadILByte();
-            }
-            else if (ldInstructionSize == 3 && opcode is ILOpcode.ldloc or ILOpcode.ldarg)
-            {
-                reader.ReadILUInt16();
+                constant = equality.Value ? 1 : 0;
             }
             else
             {
-                return false;
-            }
-
-            if ((flags[reader.Offset] & OpcodeFlags.BasicBlockStart) != 0)
-                return false;
+                var knownType = (TypeDesc)methodIL.GetObject(analyzer.Token1);
 
-            if (expectGetType)
-            {
-                if (reader.ReadILOpcode() is not ILOpcode.callvirt and not ILOpcode.call)
+                // No value in making this work for definitions
+                if (knownType.IsGenericDefinition)
                     return false;
 
-                // We don't actually mind if this is not Object.GetType
-                reader.ReadILToken();
-
-                if ((flags[reader.Offset] & OpcodeFlags.BasicBlockStart) != 0)
+                // Dataflow runs on top of uninstantiated IL and we can't answer some questions there.
+                // Unfortunately this means dataflow will still see code that the rest of the system
+                // might have optimized away. It should not be a problem in practice.
+                if (knownType.ContainsSignatureVariables())
                     return false;
-            }
 
-            // If the ldtoken wasn't in the first position, it must be in the other
-            if (knownType == null)
-            {
-                knownType = ReadLdToken(ref reader, methodIL, flags);
-                if (knownType == null)
+                if (knownType.IsCanonicalDefinitionType(CanonicalFormKind.Any))
                     return false;
 
-                if (!ReadGetTypeFromHandle(ref reader, methodIL, flags))
+                // We don't track types without a constructed MethodTable very well.
+                if (!ConstructedEETypeNode.CreationAllowed(knownType))
                     return false;
-            }
 
-            // No value in making this work for definitions
-            if (knownType.IsGenericDefinition)
-                return false;
-
-            // Dataflow runs on top of uninstantiated IL and we can't answer some questions there.
-            // Unfortunately this means dataflow will still see code that the rest of the system
-            // might have optimized away. It should not be a problem in practice.
-            if (knownType.ContainsSignatureVariables())
-                return false;
-
-            if (knownType.IsCanonicalDefinitionType(CanonicalFormKind.Any))
-                return false;
+                if (_devirtualizationManager.CanReferenceConstructedTypeOrCanonicalFormOfType(knownType.NormalizeInstantiation()))
+                    return false;
 
-            // We don't track types without a constructed MethodTable very well.
-            if (!ConstructedEETypeNode.CreationAllowed(knownType))
-                return false;
+                constant = 0;
+            }
 
-            if (_devirtualizationManager.CanReferenceConstructedTypeOrCanonicalFormOfType(knownType.NormalizeInstantiation()))
-                return false;
+            if (analyzer.IsInequality)
+                constant ^= 1;
 
-            constant = 0;
             return true;
         }