[PATCH] D57915: [COFF, ARM64] Remove definitions for _byteswap library functions

2019-02-11 Thread Tom Tan via Phabricator via cfe-commits
TomTan added a comment. In D57915#1393510 , @thakis wrote: > In D57915#1389722 , @TomTan wrote: > > > In D57915#1389560 , @lebedev.ri > > wrote: > > > > > In D57915#1389549

[PATCH] D57915: [COFF, ARM64] Remove definitions for _byteswap library functions

2019-02-11 Thread Nico Weber via Phabricator via cfe-commits
thakis added a comment. In D57915#1389722 , @TomTan wrote: > In D57915#1389560 , @lebedev.ri > wrote: > > > In D57915#1389549 , @TomTan wrote: > > > > > Added the tests

[PATCH] D57915: [COFF, ARM64] Remove definitions for _byteswap library functions

2019-02-11 Thread Tom Tan via Phabricator via cfe-commits
This revision was automatically updated to reflect the committed changes. Closed by commit rL353740: [COFF, ARM64] Remove definitions for _byteswap library functions (authored by TomTan, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to

[PATCH] D57915: [COFF, ARM64] Remove definitions for _byteswap library functions

2019-02-07 Thread Eli Friedman via Phabricator via cfe-commits
efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. I guess we can track inlining separately, if you want to merge this quickly to unblock the Chrome build. LGTM > the former provides global declaration which seems inherited by the >

[PATCH] D57915: [COFF, ARM64] Remove definitions for _byteswap library functions

2019-02-07 Thread Tom Tan via Phabricator via cfe-commits
TomTan added a comment. In D57915#1389788 , @efriedma wrote: > I did some quick testing with MSVC; apparently it inlines the implementations > of these functions when optimizations are on. We definitely want to support > inlining these. Since these are

[PATCH] D57915: [COFF, ARM64] Remove definitions for _byteswap library functions

2019-02-07 Thread Tom Tan via Phabricator via cfe-commits
TomTan added a comment. In D57915#1389750 , @lebedev.ri wrote: > In D57915#1389722 , @TomTan wrote: > > > In D57915#1389560 , @lebedev.ri > > wrote: > > > > > In

[PATCH] D57915: [COFF, ARM64] Remove definitions for _byteswap library functions

2019-02-07 Thread Eli Friedman via Phabricator via cfe-commits
efriedma added a comment. I did some quick testing with MSVC; apparently it inlines the implementations of these functions when optimizations are on. We definitely want to support inlining these. Since these are commonly used in performance-sensitive code, I'd prefer to implement the required

[PATCH] D57915: [COFF, ARM64] Remove definitions for _byteswap library functions

2019-02-07 Thread Roman Lebedev via Phabricator via cfe-commits
lebedev.ri added a comment. In D57915#1389722 , @TomTan wrote: > In D57915#1389560 , @lebedev.ri > wrote: > > > In D57915#1389549 , @TomTan wrote: > > > > > Added the tests

[PATCH] D57915: [COFF, ARM64] Remove definitions for _byteswap library functions

2019-02-07 Thread Tom Tan via Phabricator via cfe-commits
TomTan added a comment. In D57915#1389560 , @lebedev.ri wrote: > In D57915#1389549 , @TomTan wrote: > > > Added the tests back. Clang IR should not lower these to bswap calls > > because they are global library

[PATCH] D57915: [COFF, ARM64] Remove definitions for _byteswap library functions

2019-02-07 Thread Roman Lebedev via Phabricator via cfe-commits
lebedev.ri added a comment. In D57915#1389549 , @TomTan wrote: > Added the tests back. Clang IR should not lower these to bswap calls because > they are global library functions. It might be slower to make the call to > library function than bswap, but

[PATCH] D57915: [COFF, ARM64] Remove definitions for _byteswap library functions

2019-02-07 Thread Tom Tan via Phabricator via cfe-commits
TomTan updated this revision to Diff 185849. TomTan added a comment. Added the tests back. Clang IR should not lower these to bswap calls because they are global library functions. It might be slower to make the call to library function than bswap, but this is the same for other architectures

[PATCH] D57915: [COFF, ARM64] Remove definitions for _byteswap library functions

2019-02-07 Thread Eli Friedman via Phabricator via cfe-commits
efriedma added a comment. Should clang IR generation be lowering these to bswap calls anyway? Even if the function technically exists in the ucrt, it's going to be pretty slow to call it. Please leave the tests; fix the CHECK lines, if necessary, but we should still check it compiles.

[PATCH] D57915: [COFF, ARM64] Remove definitions for _byteswap library functions

2019-02-07 Thread Tom Tan via Phabricator via cfe-commits
TomTan created this revision. TomTan added reviewers: mgrang, efriedma, mstorsjo. Herald added subscribers: cfe-commits, kristof.beyls, javed.absar. Herald added a project: clang. _byteswap_* functions are are implemented in below file as normal function from libucrt.lib and declared in stdlib.h.