{"action":"created","issue":{"url":"https://api.github.com/repos/JohnLangford/vowpal_wabbit/issues/870","labels_url":"https://api.github.com/repos/JohnLangford/vowpal_wabbit/issues/870/labels{/name}","comments_url":"https://api.github.com/repos/JohnLangford/vowpal_wabbit/issues/870/comments","events_url":"https://api.github.com/repos/JohnLangford/vowpal_wabbit/issues/870/events","html_url":"https://github.com/JohnLangford/vowpal_wabbit/pull/870","id":119205274,"number":870,"title":"Reduce warnings and enable strict aliasing","user":{"login":"willnewton","id":93721,"avatar_url":"https://avatars.githubusercontent.com/u/93721?v=3","gravatar_id":"","url":"https://api.github.com/users/willnewton","html_url":"https://github.com/willnewton","followers_url":"https://api.github.com/users/willnewton/followers","following_url":"https://api.github.com/users/willnewton/following{/other_user}","gists_url":"https://api.github.com/users/willnewton/gists{/gist_id}","starred_url":"https://api.github.com/users/willnewton/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/willnewton/subscriptions","organizations_url":"https://api.github.com/users/willnewton/orgs","repos_url":"https://api.github.com/users/willnewton/repos","events_url":"https://api.github.com/users/willnewton/events{/privacy}","received_events_url":"https://api.github.com/users/willnewton/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":6,"created_at":"2015-11-27T14:33:32Z","updated_at":"2015-12-03T12:42:36Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/JohnLangford/vowpal_wabbit/pulls/870","html_url":"https://github.com/JohnLangford/vowpal_wabbit/pull/870","diff_url":"https://github.com/JohnLangford/vowpal_wabbit/pull/870.diff","patch_url":"https://github.com/JohnLangford/vowpal_wabbit/pull/870.patch"},"body":"\r\nThese changes reduce the levels of warnings that can be generated from the headers. These are only seen with --enable-nitpick or if the headers are included in a source base that has those some of those additional warnings enabled.\r\n\r\nIn the process it fixes the aliasing warnings and removes -fno-strict-aliasing. The latter is a behavioural change which should provide potentially better optimization of the code but may cause bugs if subtle alias issues are present in the code. The tests still pass and I went back into the git history but couldn't find the precise reason why the flag was added in the first place.\r\n"},"comment":{"url":"https://api.github.com/repos/JohnLangford/vowpal_wabbit/issues/comments/161622611","html_url":"https://github.com/JohnLangford/vowpal_wabbit/pull/870#issuecomment-161622611","issue_url":"https://api.github.com/repos/JohnLangford/vowpal_wabbit/issues/870","id":161622611,"user":{"login":"willnewton","id":93721,"avatar_url":"https://avatars.githubusercontent.com/u/93721?v=3","gravatar_id":"","url":"https://api.github.com/users/willnewton","html_url":"https://github.com/willnewton","followers_url":"https://api.github.com/users/willnewton/followers","following_url":"https://api.github.com/users/willnewton/following{/other_user}","gists_url":"https://api.github.com/users/willnewton/gists{/gist_id}","starred_url":"https://api.github.com/users/willnewton/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/willnewton/subscriptions","organizations_url":"https://api.github.com/users/willnewton/orgs","repos_url":"https://api.github.com/users/willnewton/repos","events_url":"https://api.github.com/users/willnewton/events{/privacy}","received_events_url":"https://api.github.com/users/willnewton/received_events","type":"User","site_admin":false},"created_at":"2015-12-03T12:42:36Z","updated_at":"2015-12-03T12:42:36Z","body":"No, I think that would work too. It also seems to compile to the same thing.\n\nOn Thu, Dec 3, 2015 at 12:35 PM, John <notifications@github.com> wrote:\n\n> Do you have an objection to using reinterpret_cast?\n>\n> -John\n>\n> On Thu, Dec 3, 2015 at 7:20 AM, Will Newton <notifications@github.com>\n>\n> wrote:\n>\n> > Wikipedia is a little unclear and I am not convinced it is authoritative\n> on\n> > this topic (although I am not sure who is!). Type-punning via a union\n> > explicitly works with gcc so it seems fine to use that approach, although\n> > it does appear that, for example, Sun CC will miscompile this type of\n> code:\n> >\n> > http://blog.qt.io/blog/2011/06/10/type-punning-and-strict-aliasing/\n> >\n> > I believe the movs are in order to get the data into the right place for\n> > the calling convention and will be optimized out if the function is\n> > inlined. For the compilers I have the union type punning and memcpy\n> > approaches produce exactly the same code but I have respun the branch\n> with\n> > the type punning approach.\n> >\n> >\n> >\n> > On Wed, Dec 2, 2015 at 2:16 PM, John <notifications@github.com> wrote:\n> >\n> > > Wikipedia suggests otherwise:\n> > > https://en.wikipedia.org/wiki/Type_punning\n> > > in the sense that using type punning is explicitly allowed in some ways\n> > > (you just need to be careful). Type punning seems better than the\n> > assembly\n> > > above since all the 'mov' operations are dropped.\n> > >\n> > > -John\n> > >\n> > >\n> > > On Tue, Dec 1, 2015 at 6:15 AM, Will Newton will.newton@gmail.com\n> > > [vowpal_wabbit] <vowpal_wabbit@yahoogroups.com> wrote:\n> > >\n> > > >\n> > > >\n> > > >\n> > > > I agree the memcpy looks like a pretty terrible idea for performance\n> > but\n> > > > it actually works out ok. Essentially the compiler can see that what\n> we\n> > > are\n> > > > doing is:\n> > > >\n> > > > memcpy(4 byte aligned ptr, 4 byte aligned ptr, 4 bytes)\n> > > >\n> > > > and optimize accordingly. For example on x86_64 at -O2 gcc 5.1\n> > generates:\n> > > >\n> > > > float_to_bits:\n> > > > movd %xmm0, %eax\n> > > > ret\n> > > > bits_to_float:\n> > > > movl %edi, -4(%rsp)\n> > > > movss -4(%rsp), %xmm0\n> > > > ret\n> > > >\n> > > > And similarly on ARM gcc 4.9 at -O2:\n> > > >\n> > > > float_to_bits:\n> > > > fmrs r0, s0 @ int\n> > > > bx lr\n> > > > bits_to_float:\n> > > > fmsr s0, r0\n> > > > bx lr\n> > > >\n> > > > It would be possible to rewrite with the union trick but my\n> > understanding\n> > > > is whilst the union trick is widely supported in compilers it is\n> > strictly\n> > > > undefined behaviour by the C++ standard to use a union in that\n> fashion.\n> > > >\n> > > >\n> > > >\n> > > > On Mon, Nov 30, 2015 at 8:29 PM, John Langford jl@hunch.net\n> > > > [vowpal_wabbit] <vowpal_wabbit@yahoogroups.com> wrote:\n> > > >\n> > > >>\n> > > >>\n> > > >> This seems good, except that quake_inv_sqrt is performance sensitive\n> > and\n> > > >> hence the memcpy is pretty undesirable. It looks like the correct\n> and\n> > > >> perfomant method for handling this is via type-punning using a union\n> > > type.\n> > > >> Can you create inline functions doing that instead? After that, I'll\n> > > merge.\n> > > >>\n> > > >> I have no recollection where no-strict-aliasing came in or why.\n> Let's\n> > > >> turn it off and see if anything breaks.\n> > > >>\n> > > >> -John\n> > > >>\n> > > >> On Fri, Nov 27, 2015 at 9:33 AM, Will Newton <\n> > notifications@github.com>\n> > > >> wrote:\n> > > >>\n> > > >>> These changes reduce the levels of warnings that can be generated\n> > from\n> > > >>> the headers. These are only seen with --enable-nitpick or if the\n> > > headers\n> > > >>> are included in a source base that has those some of those\n> additional\n> > > >>> warnings enabled.\n> > > >>>\n> > > >>> In the process it fixes the aliasing warnings and removes\n> > > >>> -fno-strict-aliasing. The latter is a behavioural change which\n> should\n> > > >>> provide potentially better optimization of the code but may cause\n> > bugs\n> > > if\n> > > >>> subtle alias issues are present in the code. The tests still pass\n> > and I\n> > > >>> went back into the git history but couldn't find the precise reason\n> > > why the\n> > > >>> flag was added in the first place.\n> > > >>> ------------------------------\n> > > >>> You can view, comment on, or merge this pull request online at:\n> > > >>>\n> > > >>> https://github.com/JohnLangford/vowpal_wabbit/pull/870\n> > > >>> Commit Summary\n> > > >>>\n> > > >>> - Fix spelling of -Wno-unused-local-typedefs\n> > > >>> - Fix some warnings in the headers\n> > > >>> - Fix float to int aliasing warnings\n> > > >>> - Fix parser aliasing warning\n> > > >>> - Turn off -fno-strict-aliasing\n> > > >>>\n> > > >>> File Changes\n> > > >>>\n> > >\n> > > >>> - *M* Makefile.am\n> > > >>> <\n> https://github.com/JohnLangford/vowpal_wabbit/pull/870/files#diff-0\n> > >\n> > > >>> (1)\n> > > >>> - *M* vowpalwabbit/Makefile.am\n> > > >>> <\n> https://github.com/JohnLangford/vowpal_wabbit/pull/870/files#diff-1\n> > >\n> > > >>> (4)\n> > > >>> - *M* vowpalwabbit/example.h\n> > > >>> <\n> https://github.com/JohnLangford/vowpal_wabbit/pull/870/files#diff-2\n> > >\n> > > >>> (2)\n> > > >>> - *A* vowpalwabbit/floatbits.h\n> > > >>> <\n> https://github.com/JohnLangford/vowpal_wabbit/pull/870/files#diff-3\n> > >\n> > > >>> (22)\n> > > >>> - *M* vowpalwabbit/gd.cc\n> > > >>> <\n> https://github.com/JohnLangford/vowpal_wabbit/pull/870/files#diff-4\n> > >\n> > > >>> (5)\n> > > >>> - *M* vowpalwabbit/learner.h\n> > > >>> <\n> https://github.com/JohnLangford/vowpal_wabbit/pull/870/files#diff-5\n> > >\n> > > >>> (2)\n> > > >>> - *M* vowpalwabbit/main.cc\n> > > >>> <\n> https://github.com/JohnLangford/vowpal_wabbit/pull/870/files#diff-6\n> > >\n> > > >>> (1)\n> > > >>> - *M* vowpalwabbit/parse_args.h\n> > > >>> <\n> https://github.com/JohnLangford/vowpal_wabbit/pull/870/files#diff-7\n> > >\n> > > >>> (1)\n> > > >>> - *M* vowpalwabbit/parse_primitives.h\n> > > >>> <\n> https://github.com/JohnLangford/vowpal_wabbit/pull/870/files#diff-8\n> > >\n> > > >>> (5)\n> > > >>> - *M* vowpalwabbit/parser.cc\n> > > >>> <\n> https://github.com/JohnLangford/vowpal_wabbit/pull/870/files#diff-9\n> > >\n> > > >>> (5)\n> > > >>> - *M* vowpalwabbit/rand48.cc\n> > > >>> <\n> > https://github.com/JohnLangford/vowpal_wabbit/pull/870/files#diff-10>\n> > > >>> (3)\n> > > >>> - *M* vowpalwabbit/v_array.h\n> > > >>> <\n> > https://github.com/JohnLangford/vowpal_wabbit/pull/870/files#diff-11>\n> > > >>> (2)\n> > > >>> - *M* vowpalwabbit/vw.h\n> > > >>> <\n> > https://github.com/JohnLangford/vowpal_wabbit/pull/870/files#diff-12>\n> > > >>> (2)\n> > > >>>\n> > > >>> Patch Links:\n> > > >>>\n> > > >>> - https://github.com/JohnLangford/vowpal_wabbit/pull/870.patch\n> > > >>> - https://github.com/JohnLangford/vowpal_wabbit/pull/870.diff\n> > > >>>\n> > > >>> —\n> > > >>> Reply to this email directly or view it on GitHub\n> > > >>> <https://github.com/JohnLangford/vowpal_wabbit/pull/870>.\n> > > >>>\n> > > >>\n> > > >>\n> > > > __._,_.___\n> > > > ------------------------------\n> > > > Posted by: Will Newton <will.newton@gmail.com>\n> > > > ------------------------------\n> > > > Reply via web post\n> > > > <\n> > >\n> >\n> https://groups.yahoo.com/neo/groups/vowpal_wabbit/conversations/messages/5666;_ylc=X3oDMTJxZ3ZmMXByBF9TAzk3MzU5NzE0BGdycElkAzczNzEzNTMzBGdycHNwSWQDMTcwNzI4MTk0MgRtc2dJZAM1NjY2BHNlYwNmdHIEc2xrA3JwbHkEc3RpbWUDMTQ0OTAwMTc3MA--?act=reply&messageNum=5666\n> > > >\n> > > > • Reply to sender\n> > > > <\n> > >\n> >\n> will.newton@gmail.com?subject=Re%3A%20%5Bvowpal_wabbit%5D%20Reduce%20warnings%20and%20enable%20strict%20aliasing%20%28%23870%29\n> > > >\n> > > > • Reply to group\n> > > > <\n> > >\n> >\n> vowpal_wabbit@yahoogroups.com?subject=Re%3A%20%5Bvowpal_wabbit%5D%20Reduce%20warnings%20and%20enable%20strict%20aliasing%20%28%23870%29\n> > > >\n> > > > • Start a New Topic\n> > > > <\n> > >\n> >\n> https://groups.yahoo.com/neo/groups/vowpal_wabbit/conversations/newtopic;_ylc=X3oDMTJmYWk2cWp0BF9TAzk3MzU5NzE0BGdycElkAzczNzEzNTMzBGdycHNwSWQDMTcwNzI4MTk0MgRzZWMDZnRyBHNsawNudHBjBHN0aW1lAzE0NDkwMDE3NzA-\n> > > >\n> > > > • Messages in this topic\n> > > > <\n> > >\n> >\n> https://groups.yahoo.com/neo/groups/vowpal_wabbit/conversations/topics/5661;_ylc=X3oDMTM1Ymw4b2xvBF9TAzk3MzU5NzE0BGdycElkAzczNzEzNTMzBGdycHNwSWQDMTcwNzI4MTk0MgRtc2dJZAM1NjY2BHNlYwNmdHIEc2xrA3Z0cGMEc3RpbWUDMTQ0OTAwMTc3MAR0cGNJZAM1NjYx\n> > > >\n> > > > (2)\n> > > > Visit Your Group\n> > > > <\n> > >\n> >\n> https://groups.yahoo.com/neo/groups/vowpal_wabbit/info;_ylc=X3oDMTJmc3Q4cmk5BF9TAzk3MzU5NzE0BGdycElkAzczNzEzNTMzBGdycHNwSWQDMTcwNzI4MTk0MgRzZWMDdnRsBHNsawN2Z2hwBHN0aW1lAzE0NDkwMDE3NzA-\n> > > >\n> > > >\n> > > > - New Members\n> > > > <\n> > >\n> >\n> https://groups.yahoo.com/neo/groups/vowpal_wabbit/members/all;_ylc=X3oDMTJnNWJwaDRoBF9TAzk3MzU5NzE0BGdycElkAzczNzEzNTMzBGdycHNwSWQDMTcwNzI4MTk0MgRzZWMDdnRsBHNsawN2bWJycwRzdGltZQMxNDQ5MDAxNzcw\n> > > >\n> > > > 3\n> > > >\n> > > > [image: Yahoo! Groups]\n> > > > <\n> > >\n> >\n> https://groups.yahoo.com/neo;_ylc=X3oDMTJlNnBqdGNoBF9TAzk3NDc2NTkwBGdycElkAzczNzEzNTMzBGdycHNwSWQDMTcwNzI4MTk0MgRzZWMDZnRyBHNsawNnZnAEc3RpbWUDMTQ0OTAwMTc3MA--\n> > > >\n> > > > • Privacy <\n> https://info.yahoo.com/privacy/us/yahoo/groups/details.html\n> > >\n> > > •\n> > > > Unsubscribe\n> > > > <vowpal_wabbit-unsubscribe@yahoogroups.com?subject=Unsubscribe> •\n> > Terms\n> > > > of Use <https://info.yahoo.com/legal/us/yahoo/utos/terms/>\n> > > >\n> > > > .\n> > > >\n> > > > __,_._,___\n> > > >\n> > >\n> > > —\n> > > Reply to this email directly or view it on GitHub\n> > > <\n> >\n> https://github.com/JohnLangford/vowpal_wabbit/pull/870#issuecomment-161309629\n> > >\n> > > .\n> >\n> > >\n> >\n> > —\n> > Reply to this email directly or view it on GitHub\n> > <\n> https://github.com/JohnLangford/vowpal_wabbit/pull/870#issuecomment-161618750\n> >\n> > .\n>\n> >\n>\n> —\n> Reply to this email directly or view it on GitHub\n> <https://github.com/JohnLangford/vowpal_wabbit/pull/870#issuecomment-161621389>\n> .\n>\n"}}