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

[Tom Tan via Phabricator via cfe-commits]

TomTan added a comment.

In D57915#1389750 <https://reviews.llvm.org/D57915#1389750>, @lebedev.ri wrote:

> In D57915#1389722 <https://reviews.llvm.org/D57915#1389722>, @TomTan wrote:
>
> > In D57915#1389560 <https://reviews.llvm.org/D57915#1389560>, @lebedev.ri wrote:
> >
> > > In D57915#1389549 <https://reviews.llvm.org/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 this is the same for other architectures supported by Windows. And just redefine global library function triggers link error in some scenarios.
> > >
> > >
> > > I think there is some deeper reasoning is being omitted here.
> > >  //Why// does the fact that those are "global library functions" bans clang from lowering them into IR?
> > >  (and thus, "prevents" LLVM form doing optimizations)
> >
> >
> > The current issue could be caused by the implementation of comdat selection in LLD as below which provides more strict conflict check than MSVC link does.
> >
> > These `_bytewap_*` in `intrin.h` were not intended as optimization, instead, it is expected to be consistent with declarations in `stdlib.h`. For optimization, it makes sense to enable it for all architectures supported by Windows, and make sure it works with LLD.
> >
> > https://github.com/llvm-mirror/lld/blob/b6584c3fab115aa46dad27681af7eb3d4e5d2b35/COFF/InputFiles.cpp#L494
>
>
> These functions are also listed in https://github.com/llvm-mirror/clang/blob/c18e7e9007970a3105617f03bc9d1de89fa1a3e1/include/clang/Basic/Builtins.def#L773-L775
>  Are you sure they shouldn't be simply dropped from `lib/Headers/intrin.h`?
>
> I notice they were just added in D56685 <https://reviews.llvm.org/D56685>, but that review has no commit message, so the problem it addressed is not documented..
>  So as-is this all looks a bit like hand-waving //to me//.


The list in https://github.com/llvm-mirror/clang/blob/c18e7e9007970a3105617f03bc9d1de89fa1a3e1/include/clang/Basic/Builtins.def#L773-L775 doesn't require any implementation on Clang side,it provides below warning if they are not declared (include `intrin.h` or `stdlib.h`) before using. So it would not be affected.

test.cpp(3,12):  warning: implicitly declaring library function '_byteswap_ushort' with type 'unsigned short (unsigned short)' [-Wimplicit-function-declaration]

  return _byteswap_ushort(42);
         ^

test.cpp(3,12):  note: include the header <stdlib.h> or explicitly provide a declaration for '_byteswap_ushort'


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D57915/new/

https://reviews.llvm.org/D57915