{"action":"created","issue":{"url":"https://api.github.com/repos/dotnet/coreclr/issues/6401","repository_url":"https://api.github.com/repos/dotnet/coreclr","labels_url":"https://api.github.com/repos/dotnet/coreclr/issues/6401/labels{/name}","comments_url":"https://api.github.com/repos/dotnet/coreclr/issues/6401/comments","events_url":"https://api.github.com/repos/dotnet/coreclr/issues/6401/events","html_url":"https://github.com/dotnet/coreclr/pull/6401","id":166947403,"number":6401,"title":"Jit: fix issues with optMethodFlags","user":{"login":"AndyAyersMS","id":10121823,"avatar_url":"https://avatars.githubusercontent.com/u/10121823?v=3","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},"labels":[{"url":"https://api.github.com/repos/dotnet/coreclr/labels/cla-already-signed","name":"cla-already-signed","color":"009800"}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":1,"created_at":"2016-07-22T00:33:36Z","updated_at":"2016-07-22T00:34:55Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/dotnet/coreclr/pulls/6401","html_url":"https://github.com/dotnet/coreclr/pull/6401","diff_url":"https://github.com/dotnet/coreclr/pull/6401.diff","patch_url":"https://github.com/dotnet/coreclr/pull/6401.patch"},"body":"Closes #6368.\r\n\r\noptMethodFlags is a member field of the Compiler object. It is used\r\nduring importation to flag IR that might benefit from earlyProp, and\r\nused in earlyProp to skip execution if nothing is flagged. However,\r\nthere were a number of issues with the implementation.\r\n\r\nFirst, the value was never initialized. So in CHK/DBG builds, it would\r\nget set to 0xFFFFFFFF by the default allocator fill. In RELEASE builds\r\nthe value would be randomly set. This randomness could easily lead to\r\nnondeterministic codegen in RELEASE. More on this subsequently.\r\n\r\nSecond, the value was not propagated during inlining. So if interesting\r\nconstructs only entered the root method via inlines, they potentially\r\nmight not trigger earlyProp.\r\n\r\nThird, not all interesting constructs were flagged.\r\n\r\nThe JIT ORs flag values into optMethodFlags as it imports constructs\r\nthat should trigger earlyProp. It also re-uses the same compiler instance\r\nin most cases. So, relatively quickly, even in release, all the relevant\r\nflags end up set and earlyProp will always be run. Hence the window\r\nfor observing the nondeterministic is was limited to the first few\r\nmethods jitted by each compiler instance.\r\n\r\nSo to sum up: in DBG/CHK, early prop always runs, regardless of need.\r\nIn RELEASE, it runs unpredictably for the first few methods, then always\r\nruns, regardless of need.\r\n\r\nTo see the nondeterminism in RELEASE, early methods would have to be free\r\nof interesting constructs but pull them in via inlining (or contain\r\ninteresting constructs not currently flagged as such during importation).\r\nThis set of circumstances is evidently rare enough that the nondeterminism\r\nhas not been noted. Also, it is quite unlikely to lead to bad codegen,\r\njust missed opportunities.\r\n\r\nWe might have caught this if we had reliable RELEASE/CHK diffing ready;\r\nsee for instance #5063.\r\n\r\nThe fix is to initialize optMethodFlags to zero in compInit, and merge in\r\nthe inlinee's copy during fgInsertInlineeBlocks. Logging tracks when the\r\ninlinee's copy causes a flag update.\r\n\r\nThis was tricky to test for diffs because in CHK all that should happen is\r\nthat we run earlyProp less often. So any diff in CHK is a missed case of\r\nflagging during importation. I found once such case via SPMI where the array\r\nindex is a constant, and added constant indices to the flagging set.\r\n\r\nBecause I also set the block flag, this both caused earlyProp to run\r\nand also look at that block; the latter has triggered a fair number of\r\ndiffs, almost all of them beneficial (0.42% improvement in SPMI), winners\r\noutnumber losers by about 50:1.\r\n\r\nA typical example of a diff is that the null check below is removed.\r\n\r\n```asm\r\n       mov      rcx, qword ptr [(reloc)]\r\n       mov      edx, 4\r\n       call     CORINFO_HELP_NEWARR_1_VC\r\n**     mov      edx, dword ptr [rax+8]\r\n       mov      byte  ptr [rax+16], 255\r\n       mov      byte  ptr [rax+17], 254\r\n```\r\n\r\nWith this change in place, earlyProp how runs selectively based on need in\r\nall configurations. We're still running earlyProp in all the cases where it\r\ncan make a difference, and finding more improvements when it does run, but\r\nwe're not running it in cases where it can't."},"comment":{"url":"https://api.github.com/repos/dotnet/coreclr/issues/comments/234424292","html_url":"https://github.com/dotnet/coreclr/pull/6401#issuecomment-234424292","issue_url":"https://api.github.com/repos/dotnet/coreclr/issues/6401","id":234424292,"user":{"login":"AndyAyersMS","id":10121823,"avatar_url":"https://avatars.githubusercontent.com/u/10121823?v=3","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":"2016-07-22T00:34:55Z","updated_at":"2016-07-22T00:34:55Z","body":"Addtional CQ/CS/TP measurements underway; will update this with results when I have them.\r\n\r\n@erozen or @JosephTremoulet PTAL; cc @dotnet/jit-contrib "}}
{"actor":{"display_login":"AndyAyersMS"}}