{"action":"created","issue":{"url":"https://api.github.com/repos/dotnet/coreclr/issues/27167","repository_url":"https://api.github.com/repos/dotnet/coreclr","labels_url":"https://api.github.com/repos/dotnet/coreclr/issues/27167/labels{/name}","comments_url":"https://api.github.com/repos/dotnet/coreclr/issues/27167/comments","events_url":"https://api.github.com/repos/dotnet/coreclr/issues/27167/events","html_url":"https://github.com/dotnet/coreclr/pull/27167","id":506259815,"node_id":"MDExOlB1bGxSZXF1ZXN0MzI3NTE0MTYz","number":27167,"title":"Expand BBJ_RETURN blocks with bool conditions","user":{"login":"EgorBo","id":523221,"node_id":"MDQ6VXNlcjUyMzIyMQ==","avatar_url":"https://avatars0.githubusercontent.com/u/523221?v=4","gravatar_id":"","url":"https://api.github.com/users/EgorBo","html_url":"https://github.com/EgorBo","followers_url":"https://api.github.com/users/EgorBo/followers","following_url":"https://api.github.com/users/EgorBo/following{/other_user}","gists_url":"https://api.github.com/users/EgorBo/gists{/gist_id}","starred_url":"https://api.github.com/users/EgorBo/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/EgorBo/subscriptions","organizations_url":"https://api.github.com/users/EgorBo/orgs","repos_url":"https://api.github.com/users/EgorBo/repos","events_url":"https://api.github.com/users/EgorBo/events{/privacy}","received_events_url":"https://api.github.com/users/EgorBo/received_events","type":"User","site_admin":false},"labels":[{"id":196940911,"node_id":"MDU6TGFiZWwxOTY5NDA5MTE=","url":"https://api.github.com/repos/dotnet/coreclr/labels/area-CodeGen","name":"area-CodeGen","color":"d4c5f9","default":false}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":1,"created_at":"2019-10-13T01:23:57Z","updated_at":"2019-10-15T17:11:33Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/dotnet/coreclr/pulls/27167","html_url":"https://github.com/dotnet/coreclr/pull/27167","diff_url":"https://github.com/dotnet/coreclr/pull/27167.diff","patch_url":"https://github.com/dotnet/coreclr/pull/27167.patch"},"body":"This PR consists of:\r\n1) Recognize `GT_NE` early in the `importer.cpp` (roslyn replaces `x!=0` with `(uint)x>0` and it messes up some optimizations) - [jit-diff](https://gist.github.com/EgorBo/e741488f563ac44f5322a3d2fda5bfa0)\r\n2) Don't replace with `GT_NE` if `GT_ARR_LENGTH` is around - it removes a bound check for [this case](https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABAJgEYBYAKGIGYACMhgYQYG8aHuGuf6m5JAwCWAOwwMAchAwB1aAGtxAcwAU4jAG0Aug2xQo2AJ4BKDnx48RAMwZq1AV02mDR4wDoAMjDEqMABYMAHwMAAymllbRxADs+oYmWmE6ANxRVnHh6dRWAL5Rhbn8jMRCohIMClDKfhoSugnu5pzF0aJ2amEMADz2zhKuiZ4+foGRbe388W5JKTlTTPFhCzwF1HlAA). This change has 0 diffs for BCL in jit-diff and looks more like a hack and needs to be addressed in the rangecheck instead I guess but it does fix that [case](https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA0AXEBDAzgWwB8ABAJgEYBYAKGIGYACMhgYQYG8aHuGuf6m5JAwCWAOwwMAchAwB1aAGtxAcwAU4jAG0Aug2xQo2AJ4BKDnx48RAMwZq1AV02mDR4wDoAMjDEqMABYMAHwMAAymllbRxADs+oYmWmE6ANxRVnHh6dRWAL5Rhbn8jMRCohIMClDKfhoSugnu5pzF0aJ2amEMADz2zhKuiZ4+foGRbe388W5JKTlTTPFhCzwF1HlAA).\r\n3) Expand `BBJ_RETURN` basic blocks with boolean conditions, let me explain it via a pic:\r\n\r\n![image](https://user-images.githubusercontent.com/523221/66714025-ed734d00-edb9-11e9-85a2-ddb8b3d2e39b.png)\r\n\r\nSo it basically de-optimizes the control flow (optimized by Roslyn) but makes it a bit more friendly for optimizations, e.g. now both `BB1` and `BB2` jump into the same block and can be now handled by the [optOptimizeBools](https://github.com/dotnet/coreclr/blob/fd0ca99c4214538de6f821db6a38c9b6aca5ee62/src/jit/optimizer.cpp#L8851) optimizations, e.g.:\r\n```csharp\r\nstatic bool AreZero(int x, int y)\r\n{\r\n    return x == 0 && y == 0;\r\n}\r\n```\r\nWas:\r\n```asm\r\nG_M22205_IG01:\r\nG_M22205_IG02:\r\n       test     ecx, ecx\r\n       jne      SHORT G_M22205_IG05\r\nG_M22205_IG03:\t\t;; bbWeight=0.50\r\n       test     edx, edx\r\n       sete     al\r\n       movzx    rax, al\r\nG_M22205_IG04:\t\t;; bbWeight=0.50\r\n       ret      \r\nG_M22205_IG05:\t\t;; bbWeight=0.50\r\n       xor      eax, eax\r\nG_M22205_IG06:\t\t;; bbWeight=0.50\r\n       ret      \r\n; Total bytes of code: 16\r\n```\r\nNow (without `optMergeBoolReturns`):\r\n```asm\r\nG_M22205_IG01:\r\nG_M22205_IG02:\r\n       or       edx, ecx\r\n       jne      SHORT G_M22205_IG05\r\nG_M22205_IG03:\r\n       mov      eax, 1\r\nG_M22205_IG04:\r\n       ret      \r\nG_M22205_IG05:\r\n       xor      eax, eax\r\nG_M22205_IG06:\r\n       ret   \r\n```\r\nWith `optMergeBoolReturns`:\r\n```asm\r\nG_M22205_IG01:\r\nG_M22205_IG02:\r\n       or       edx, ecx\r\n       sete    al\r\n       movzx    rax, al\r\nG_M22205_IG03:\r\n       ret      \r\n```\r\n\r\nSo my de-optimization basically helped `optOptimizeBools` and now it's easier to go further and replace it with just `return x | y != 0` without jumps (see https://github.com/dotnet/coreclr/issues/27148). \r\n\r\n![image](https://user-images.githubusercontent.com/523221/66721874-ed9e3780-ee0f-11e9-829b-752206d71759.png)\r\n\r\nThe `fgExpandBoolReturns` and `optMergeBoolReturns` impl are just proof of concept so let me know please if I am moving in a wrong direction 🙂 \r\n\r\nJit diff:\r\n```\r\nFound 76 files with textual diffs.\r\n\r\nSummary:\r\n(Lower is better)\r\n\r\nTotal bytes of diff: -4452 (-0.01% of base)\r\n    diff is an improvement.\r\n\r\nTop file regressions by size (bytes):\r\n          59 : System.Private.DataContractSerialization.dasm (0.01% of base)\r\n           7 : System.Net.Requests.dasm (0.01% of base)\r\n           5 : System.Reflection.Metadata.dasm (0.00% of base)\r\n           2 : System.Security.Permissions.dasm (0.01% of base)\r\n           1 : NuGet.DependencyResolver.Core.dasm (0.00% of base)\r\n\r\nTop file improvements by size (bytes):\r\n        -665 : Microsoft.CodeAnalysis.CSharp.dasm (-0.02% of base)\r\n        -359 : System.Private.CoreLib.dasm (-0.01% of base)\r\n        -318 : System.Collections.dasm (-0.06% of base)\r\n        -310 : Microsoft.CodeAnalysis.dasm (-0.02% of base)\r\n        -282 : System.Private.Xml.dasm (-0.01% of base)\r\n\r\n74 total files with size differences (64 improved, 10 regressed), 55 unchanged.\r\n\r\nTop method regressions by size (bytes):\r\n          94 (16.52% of base) : System.Private.DataContractSerialization.dasm - System.Xml.XmlBufferReader:Equals2(int,int,ref):bool:this (3 methods)\r\n          27 (21.95% of base) : Microsoft.CodeAnalysis.CSharp.dasm - Microsoft.CodeAnalysis.CSharp.CodeGen.CodeGenerator:FieldLoadMustUseRef(ref):bool\r\n          18 ( 5.75% of base) : System.Reflection.Metadata.dasm - System.Reflection.Metadata.MetadataReader:ImplementsRedirectedInterface(struct,byref):bool:this\r\n          16 ( 1.48% of base) : Microsoft.CodeAnalysis.CSharp.dasm - Microsoft.CodeAnalysis.CSharp.Symbol:MatchAttributeTarget(ref,short,ref,ref):bool\r\n          15 ( 2.60% of base) : System.Reflection.Metadata.dasm - System.Reflection.Metadata.Ecma335.BlobHeap:DocumentNameEquals(struct,ref,bool):bool:this\r\n\r\nTop method improvements by size (bytes):\r\n         -95 (-2.45% of base) : Microsoft.CodeAnalysis.dasm - Roslyn.Utilities.EnumerableExtensions:IsEmpty(ref):bool (36 methods)\r\n         -56 (-2.35% of base) : Microsoft.CodeAnalysis.dasm - Microsoft.CodeAnalysis.ImmutableArrayExtensions:SetEquals(struct,struct,ref):bool (7 methods)\r\n         -50 (-12.02% of base) : System.Private.CoreLib.dasm - System.IO.PathInternal:IsPartiallyQualified(struct):bool (2 methods)\r\n         -41 (-9.43% of base) : Microsoft.CodeAnalysis.CSharp.dasm - Microsoft.CodeAnalysis.CSharp.ConversionsBase:HasImplicitConstantExpressionConversion(ref,ref):bool\r\n         -41 (-6.47% of base) : NuGet.Versioning.dasm - NuGet.Versioning.FloatRange:Satisfies(ref):bool:this\r\n\r\nTop method regressions by size (percentage):\r\n          27 (21.95% of base) : Microsoft.CodeAnalysis.CSharp.dasm - Microsoft.CodeAnalysis.CSharp.CodeGen.CodeGenerator:FieldLoadMustUseRef(ref):bool\r\n          94 (16.52% of base) : System.Private.DataContractSerialization.dasm - System.Xml.XmlBufferReader:Equals2(int,int,ref):bool:this (3 methods)\r\n           2 (10.53% of base) : Microsoft.CodeAnalysis.dasm - Microsoft.CodeAnalysis.UnicodeCharacterUtilities:IsCombiningChar(int):bool\r\n           2 (10.53% of base) : Microsoft.CodeAnalysis.dasm - Microsoft.CodeAnalysis.PrimitiveTypeCodeExtensions:Is64BitIntegral(int):bool\r\n\r\nTop method improvements by size (percentage):\r\n         -15 (-33.33% of base) : System.Net.Http.dasm - System.Net.Http.WinInetProxyHelper:get_AutoSettingsUsed():bool:this\r\n         -12 (-28.57% of base) : System.CommandLine.dasm - <>c:<GetUnreadOptionNames>b__13_0(ref):bool:this\r\n         -12 (-28.57% of base) : System.CommandLine.dasm - <>c:<GetUnreadParameters>b__14_0(ref):bool:this\r\n         -15 (-27.27% of base) : Newtonsoft.Json.dasm - Newtonsoft.Json.Serialization.JsonArrayContract:get_HasParameterizedCreatorInternal():bool:this\r\n         -15 (-27.27% of base) : Newtonsoft.Json.dasm - Newtonsoft.Json.Serialization.JsonDictionaryContract:get_HasParameterizedCreatorInternal():bool:this\r\n\r\n980 total methods with size differences (638 improved, 342 regressed), 249215 unchanged.\r\n```\r\n[full diff](https://gist.github.com/EgorBo/002756411a9a69e760068bf0d72a19c2)."},"comment":{"url":"https://api.github.com/repos/dotnet/coreclr/issues/comments/542314388","html_url":"https://github.com/dotnet/coreclr/pull/27167#issuecomment-542314388","issue_url":"https://api.github.com/repos/dotnet/coreclr/issues/27167","id":542314388,"node_id":"MDEyOklzc3VlQ29tbWVudDU0MjMxNDM4OA==","user":{"login":"AndyAyersMS","id":10121823,"node_id":"MDQ6VXNlcjEwMTIxODIz","avatar_url":"https://avatars1.githubusercontent.com/u/10121823?v=4","gravatar_id":"","url":"https://api.github.com/users/AndyAyersMS","html_url":"https://github.com/AndyAyersMS","followers_url":"https://api.github.com/users/AndyAyersMS/followers","following_url":"https://api.github.com/users/AndyAyersMS/following{/other_user}","gists_url":"https://api.github.com/users/AndyAyersMS/gists{/gist_id}","starred_url":"https://api.github.com/users/AndyAyersMS/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/AndyAyersMS/subscriptions","organizations_url":"https://api.github.com/users/AndyAyersMS/orgs","repos_url":"https://api.github.com/users/AndyAyersMS/repos","events_url":"https://api.github.com/users/AndyAyersMS/events{/privacy}","received_events_url":"https://api.github.com/users/AndyAyersMS/received_events","type":"User","site_admin":false},"created_at":"2019-10-15T17:11:33Z","updated_at":"2019-10-15T17:11:33Z","author_association":"MEMBER","body":"I wonder if the expansion part of this should be moved into the \"constant value return merging\" logic that was introduced in #13792."}}
{"actor":{"display_login":"AndyAyersMS"}}