{"action":"created","issue":{"url":"https://api.github.com/repos/dotnet/coreclr/issues/7537","repository_url":"https://api.github.com/repos/dotnet/coreclr","labels_url":"https://api.github.com/repos/dotnet/coreclr/issues/7537/labels{/name}","comments_url":"https://api.github.com/repos/dotnet/coreclr/issues/7537/comments","events_url":"https://api.github.com/repos/dotnet/coreclr/issues/7537/events","html_url":"https://github.com/dotnet/coreclr/pull/7537","id":181859852,"number":7537,"title":"Fix the codegen for by-value struct args on x86.","user":{"login":"pgavlin","id":1224093,"avatar_url":"https://avatars.githubusercontent.com/u/1224093?v=3","gravatar_id":"","url":"https://api.github.com/users/pgavlin","html_url":"https://github.com/pgavlin","followers_url":"https://api.github.com/users/pgavlin/followers","following_url":"https://api.github.com/users/pgavlin/following{/other_user}","gists_url":"https://api.github.com/users/pgavlin/gists{/gist_id}","starred_url":"https://api.github.com/users/pgavlin/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/pgavlin/subscriptions","organizations_url":"https://api.github.com/users/pgavlin/orgs","repos_url":"https://api.github.com/users/pgavlin/repos","events_url":"https://api.github.com/users/pgavlin/events{/privacy}","received_events_url":"https://api.github.com/users/pgavlin/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"},{"url":"https://api.github.com/repos/dotnet/coreclr/labels/CodeGen","name":"CodeGen","color":"d4c5f9"},{"url":"https://api.github.com/repos/dotnet/coreclr/labels/x86","name":"x86","color":"fbca04"}],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":8,"created_at":"2016-10-09T02:11:35Z","updated_at":"2016-10-10T23:13:19Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/dotnet/coreclr/pulls/7537","html_url":"https://github.com/dotnet/coreclr/pull/7537","diff_url":"https://github.com/dotnet/coreclr/pull/7537.diff","patch_url":"https://github.com/dotnet/coreclr/pull/7537.patch"},"body":"Fix the codegen for by-value struct args on x86.\r\nAlthough the existing code generation for by-value struct args containing\r\nGC references on x86 was correct with respect to the layout of the data\r\nbeing passed, it was not correct with respect to the invariants necessary\r\nfor the GC info encoder.\r\n\r\nThe existing codegen for by-value struct args--whether represented by\r\n`GT_OBJ` or `GT_FIELD_LIST` nodes--takes what is essentially the\r\nfollowing approach:\r\n\r\n    sub esp, argument_size\r\n    mov [esp + offset_0], field_0\r\n    mov [esp + offset_1], field_1\r\n    ...\r\n    mov [esp + offset_n], field_n\r\n\r\nAlthough this puts the correct data in the correct locations, the\r\nemitter is unable to recognize that a GC info update is requires if any\r\nof the `mov` instructions write a GC reference to the stack: instead,\r\non x86, `push` instructions must be used to write GC references on the\r\nstack. Furthermore, the stack must be kept 4-byte aligned.\r\n\r\nThis change modifies the code generator to use `push` instructions to\r\nwrite outgoing GC references to the stack while obeying the alignment\r\ninvariant required by the GC encoding. For outgoing by-value struct\r\nargs represented by `GT_OBJ`, this results in very straightforward code:\r\n\r\n    push [val_addr]\r\n    push [val_addr + 4]\r\n    ...\r\n    push [val_addr + n]\r\n\r\nFor args represented by `GT_FIELD_LIST`, the picture is a bit more\r\ncomplicated as the stack must be kept 4-byte aligned. Handling of these\r\nargs is split into two cases: if a field list contains no GC references\r\nand is larger than 16 bytes or contains any fields that cannot be\r\ntreated as a 4-byte-aligned, int32-sized value, the argument is passed\r\nusing the `sub esp, arg_size; mov [esp + offset], field` approach.\r\nOtherwise--if the struct contains any GC references or is smaller than\r\n16 bytes and only contains fields that can be treated as slots--the\r\nargument is passed using the following approach:\r\n- If a field is a GC reference or can be treated as a 4-byte slot, the\r\n  field is stored to the stack using a `push` instruction.\r\n- Otherwise, the stack pointer is first adjusted to the next 4-byte\r\n  aligned address after the offset of the field and the field is\r\n  stored to the appropriate address using a `mov`.\r\n\r\nFor example, given the following IR:\r\n\r\n     t55 =    lclVar    bool   V06 tmp4         u:6 esi REG esi RV $40\r\n\r\n    t138 =    dconst    float  0.00000000000000000 REG mm0 $80\r\n\r\n           /--*  t138   float\r\n           +--*  t55    bool\r\n    t139 = *  <fldList> void   float at offset 4 REG NA $180\r\n\r\n           /--*  t139   void\r\n    t152 = *  putarg_stk [+0x00] void   REG NA\r\n\r\nThe backend will generate the following code:\r\n\r\n    sub      esp, 8\r\n    movss    dword ptr [esp+04H], xmm0\r\n    mov      dword ptr [esp], esi\r\n\r\nAnd given the following IR:\r\n\r\n    t13 =    lclVar    ref    V04 tmp1         u:2 esi (last use) REG esi RV $1c0\r\n\r\n    t45 =    lclVar    int    V05 tmp2         u:2 eax (last use) REG eax RV $200\r\n\r\n          /--*  t45    int\r\n          +--*  t13    ref\r\n    t46 = *  <fldList> void   int at offset 4 REG NA $240\r\n\r\nThe backend will instead generate the following:\r\n\r\n    push     eax\r\n    push     esi\r\n\r\nThese changes also produce beneficial diffs: for example, the native image\r\nfor System.Private.CoreLib.dll has diffs in ~1000 methods for an overall\r\nsize improvement of 1%.\r\n\r\nFixes #7088."},"comment":{"url":"https://api.github.com/repos/dotnet/coreclr/issues/comments/252770557","html_url":"https://github.com/dotnet/coreclr/pull/7537#issuecomment-252770557","issue_url":"https://api.github.com/repos/dotnet/coreclr/issues/7537","id":252770557,"user":{"login":"pgavlin","id":1224093,"avatar_url":"https://avatars.githubusercontent.com/u/1224093?v=3","gravatar_id":"","url":"https://api.github.com/users/pgavlin","html_url":"https://github.com/pgavlin","followers_url":"https://api.github.com/users/pgavlin/followers","following_url":"https://api.github.com/users/pgavlin/following{/other_user}","gists_url":"https://api.github.com/users/pgavlin/gists{/gist_id}","starred_url":"https://api.github.com/users/pgavlin/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/pgavlin/subscriptions","organizations_url":"https://api.github.com/users/pgavlin/orgs","repos_url":"https://api.github.com/users/pgavlin/repos","events_url":"https://api.github.com/users/pgavlin/events{/privacy}","received_events_url":"https://api.github.com/users/pgavlin/received_events","type":"User","site_admin":false},"created_at":"2016-10-10T23:13:18Z","updated_at":"2016-10-10T23:13:18Z","body":"@CarolEidt: I think I've addressed all feedback--PTAL."}}
{"actor":{"display_login":"pgavlin"}}