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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
