[PATCH] D39700: [Builtins] Do not use tailcall for Thumb1

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

In https://reviews.llvm.org/D39700#918030, @weimingz wrote:

> In https://reviews.llvm.org/D39700#917653, @peter.smith wrote:
>
> > I agree that we must avoid the 16-bit branch on Thumb1 and that the code sequence is fine for that purpose.
> >
> > I do wonder if compiler-rt should define the __aeabi_... forwarding functions in it? I would have expected the C-library to define them. For example the v6-m libgcc.a does not define the __aeabi memcpy family, the forwarding is done in newlib. Similarly Arm's proprietary compiler's C library defines __aeabi_memcpy that just falls through directly into memcpy. I'm guessing that there is some C-library that we support that does not define the __aeabi_ function?
>
>
> I looked the MUSL C library: it implements __aeabi_mem{cpy,move,set,clear}, but misses "__aeabi_memcmp". 
>  From the "C Library ABI for the ARM" [1], it only requires some __aeabi__ specific constants or typedefs in libc's header file. 
>  My understanding is, compiler-rt should define complete function sets to support other libc.
>
> [1] http://infocenter.arm.com/help/topic/com.arm.doc.ihi0039d/IHI0039D_clibabi.pdf


I'm not sure that the ABI will be that much help here. My understanding is that the toolchain is required to provide the __aeabi_ family of functions. Whether they are implemented in a runtime library or in the C-library is an implementation detail. Given that compiler-rt will need to support more than one C-library (Musl, libc, newlib etc.) it probably has to assume that none of the __aeabi_ specialisations of memcpy, memset etc. are implemented by the C-library.

Looking again at the default library search order, the C-library will be searched before compiler-rt so if the C-library does define an optimised __aeabi_ implementation it will get used in preference to the one in compiler-rt so I don't think it hurts to have these in compiler-rt.

Overall I don't have any objections to the change.


https://reviews.llvm.org/D39700