{"action":"created","issue":{"url":"https://api.github.com/repos/facebook/hhvm/issues/7348","repository_url":"https://api.github.com/repos/facebook/hhvm","labels_url":"https://api.github.com/repos/facebook/hhvm/issues/7348/labels{/name}","comments_url":"https://api.github.com/repos/facebook/hhvm/issues/7348/comments","events_url":"https://api.github.com/repos/facebook/hhvm/issues/7348/events","html_url":"https://github.com/facebook/hhvm/pull/7348","id":177390403,"number":7348,"title":"[AArch64] Fix overflowing integer multiplication for AArch64","user":{"login":"ptomsich","id":14983582,"avatar_url":"https://avatars.githubusercontent.com/u/14983582?v=3","gravatar_id":"","url":"https://api.github.com/users/ptomsich","html_url":"https://github.com/ptomsich","followers_url":"https://api.github.com/users/ptomsich/followers","following_url":"https://api.github.com/users/ptomsich/following{/other_user}","gists_url":"https://api.github.com/users/ptomsich/gists{/gist_id}","starred_url":"https://api.github.com/users/ptomsich/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ptomsich/subscriptions","organizations_url":"https://api.github.com/users/ptomsich/orgs","repos_url":"https://api.github.com/users/ptomsich/repos","events_url":"https://api.github.com/users/ptomsich/events{/privacy}","received_events_url":"https://api.github.com/users/ptomsich/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/facebook/hhvm/labels/CLA%20Signed","name":"CLA Signed","color":"009900"},{"url":"https://api.github.com/repos/facebook/hhvm/labels/GH%20Review:%20review-needed","name":"GH Review: review-needed","color":"fbca04"}],"state":"open","locked":false,"assignee":{"login":"mxw","id":1013739,"avatar_url":"https://avatars.githubusercontent.com/u/1013739?v=3","gravatar_id":"","url":"https://api.github.com/users/mxw","html_url":"https://github.com/mxw","followers_url":"https://api.github.com/users/mxw/followers","following_url":"https://api.github.com/users/mxw/following{/other_user}","gists_url":"https://api.github.com/users/mxw/gists{/gist_id}","starred_url":"https://api.github.com/users/mxw/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mxw/subscriptions","organizations_url":"https://api.github.com/users/mxw/orgs","repos_url":"https://api.github.com/users/mxw/repos","events_url":"https://api.github.com/users/mxw/events{/privacy}","received_events_url":"https://api.github.com/users/mxw/received_events","type":"User","site_admin":false},"assignees":[{"login":"mxw","id":1013739,"avatar_url":"https://avatars.githubusercontent.com/u/1013739?v=3","gravatar_id":"","url":"https://api.github.com/users/mxw","html_url":"https://github.com/mxw","followers_url":"https://api.github.com/users/mxw/followers","following_url":"https://api.github.com/users/mxw/following{/other_user}","gists_url":"https://api.github.com/users/mxw/gists{/gist_id}","starred_url":"https://api.github.com/users/mxw/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/mxw/subscriptions","organizations_url":"https://api.github.com/users/mxw/orgs","repos_url":"https://api.github.com/users/mxw/repos","events_url":"https://api.github.com/users/mxw/events{/privacy}","received_events_url":"https://api.github.com/users/mxw/received_events","type":"User","site_admin":false}],"milestone":null,"comments":19,"created_at":"2016-09-16T10:17:34Z","updated_at":"2016-09-19T18:38:49Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/facebook/hhvm/pulls/7348","html_url":"https://github.com/facebook/hhvm/pull/7348","diff_url":"https://github.com/facebook/hhvm/pull/7348.diff","patch_url":"https://github.com/facebook/hhvm/pull/7348.patch"},"body":"AArch64 does not set flags on an integer-multiply and the common\r\nway to detect an overflow on integer multiplication will only\r\nprovide EQ/NEQ flags.  To avoid taking a major penalty when\r\nchecking for overflows on integer multiplication, we make the\r\nlowering of MulIntO architecture-specific and introduce a new\r\narchitecture-independent mulint (similarily to divint) vasm-insn.\r\n\r\nThe MulIntO node is lowered into the new 'mulint' vasm-instr, which\r\nis a special kind of branching instruction (i.e. it takes a label\r\nfor the slow-path, in case an integer-overflow occurs).\r\n'mulint' is marked to clobber the condition-code register.\r\n\r\nThe recommended way to detect an overflow on integer multiplication\r\non AArch64 is:\r\n  mul\t  calculate lower 64bit of the result\r\n  smulh\t  calculate the upper (signed/sign-extended) 64bits\r\n  cmp\t  compare if the upper 64bits are the sign-extension\r\n\t      of the lower 64bits (i.e. if all bits in the\r\n \t      upper register are the same as the sign-bit in\r\n\t      the lower register.\r\n\r\nTo efficiently expand into such a sequence, the following vasm\r\ninstructions have been added:\r\n  smulh\t     upper 64bits of the 128bit result of a signed multiply\r\n\t       (could be named more nicely, but as only AArch64 is\r\n\t        using anyway...)\r\n  cmpqsign   compare if one operand is the sign-extension of the\r\n               other operand (i.e if all bits in one register are\r\n               equivalent to the sign-bit of the other)\r\n  mulint     integer-multiplication w/ overflow-checking\r\n\r\nAt the same time, the 'imul' vasm has been altered to _not_ set any\r\nflags and _not_ perform any overflow-checking.  As it's only used\r\nfrom the non-overflowing multiplication path in irlower-arith.c this\r\nseems to be safe.  We an not remove 'imul', as it will still be used\r\nto expand to lower 'MulInt'.\r\n\r\nX-AffectedTests: quick/float-overflow.php\r\nSigned-off-by: Philipp Tomsich <philipp.tomsich@theobroma-systems.com>"},"comment":{"url":"https://api.github.com/repos/facebook/hhvm/issues/comments/248082430","html_url":"https://github.com/facebook/hhvm/pull/7348#issuecomment-248082430","issue_url":"https://api.github.com/repos/facebook/hhvm/issues/7348","id":248082430,"user":{"login":"ptomsich","id":14983582,"avatar_url":"https://avatars.githubusercontent.com/u/14983582?v=3","gravatar_id":"","url":"https://api.github.com/users/ptomsich","html_url":"https://github.com/ptomsich","followers_url":"https://api.github.com/users/ptomsich/followers","following_url":"https://api.github.com/users/ptomsich/following{/other_user}","gists_url":"https://api.github.com/users/ptomsich/gists{/gist_id}","starred_url":"https://api.github.com/users/ptomsich/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ptomsich/subscriptions","organizations_url":"https://api.github.com/users/ptomsich/orgs","repos_url":"https://api.github.com/users/ptomsich/repos","events_url":"https://api.github.com/users/ptomsich/events{/privacy}","received_events_url":"https://api.github.com/users/ptomsich/received_events","type":"User","site_admin":false},"created_at":"2016-09-19T18:38:49Z","updated_at":"2016-09-19T18:38:49Z","body":"@dave-estes: Could we then agree on either redefining the imul to only generate an overflow flag and on having a separate vasm-token for non-flag-setting integer multiplication? Generating and setting all the flags on AArch64 will introduce a very high overhead, while nobody needs any flag except the overflow.\n\nThis will leave only the small detail that AArch64 would set the NZ-flags for overflow to sort out...\nWhen I started this change, irlower-arith assumed that an imul-overflow would signal as a CC_O (which would be the V-flag on AArch64), but in reality we’ll generate a CC_NE.  This could be done with a change of the interface between irlower-arith and the backends (either interrogating the backends to get the appropriate flag or by introducing an additional pseudo-flag CC_MULO which is then mapped onto CC_O or onto CC_NE depending on the backend). \n\n> On 19 Sep 2016, at 19:38, Dave Estes <notifications@github.com> wrote:\n> \n> Thanks a ton, @ptomsich <https://github.com/ptomsich>. OK, I think I see what you're getting at. Is this a good summation?\n> \n> When we finally have separate VRegs for each SF bit, then we'll have precise liveness information for each of those bits. With my approach, the forthcoming undef SF pass would then be able to selectively emit the instruction sequences to set each of those bits and would then OR them all up. With your approach we to do that much earlier using separate basic blocks (or just vasm) to set each of the bits. Your approach of using IR gives you the ability to actually move the setting of the bits closer to their uses and hopefully avoiding setting bits when a previous bit's use short-circuits the execution.\n> \n> Sounds reasonable to me. My general approach to our port has been to implement as much as possible in vasm before emitting. The same patch that we will introduce the undef SF pass with also moves a lot of the implementation of the more CISC-like vasm into lowerers from their current emitters.\n> \n> Still, I feel this is a very complicated change without any benefit to the codebase today. It lays the groundwork for a future optimization that relies on an uncertain multi-VReg SF feature. Furthermore, we don't even have a known workload code sequence that checks for overflow, negative, and zero flags from a single imul. If we had a critical workload or benchmark, then we'd have a much stronger motivation to design and implement the multi-VReg SF feature. And if that happened, then I'd totally be on board with your approach. Until then, I think setting flags while emitting and then landing the PR for the undef SF pass is the least disruptive path to provide the most benefit.\n> \n> —\n> You are receiving this because you were mentioned.\n> Reply to this email directly, view it on GitHub <https://github.com/facebook/hhvm/pull/7348#issuecomment-248064047>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AOShnv34St8fvQs9Rh8fFcPhZu0LDY2Nks5qrsipgaJpZM4J-yaF>.\n> \n\n"}}
{"actor":{"display_login":"ptomsich"}}