<div dir="ltr">On Mon, Oct 6, 2014 at 6:24 PM, Steven Wu <span dir="ltr"><<a href="mailto:stevenwu@apple.com" target="_blank">stevenwu@apple.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word">Agree. I made the patch that way because .thumb directive is separate from function declaration and it is hard to figure out whether some function is arm or thumb. <div>You can wrap #ifdef __ARM_ARCH_ISA_THUMB == 2 around the function declaration to pick DEFINE_COMPILERRT_FUNCTION or DEFINE_COMPILER_THUMB_FUNCTION to make the code cleaner, but it will make no functional change.<div>I am working on a patch to convert all arm builtins to Thumb if possible. After that, we should have only one function declaration macro in both cases.</div></div></div></blockquote><div><br></div><div>Agreed that it is no functional change. I like this suggestion, and think I will go ahead and make that change.</div><div><br></div><div>I will add that I look forward to the conversion. I will admit that I didn't spend any time to check if anything could be better optimized with the thumb instructions (as perhaps I should have).</div><div><br></div><div>Thanks for working on this!</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div><div class="h5"><div><div><blockquote type="cite"><div>On Oct 6, 2014, at 6:17 PM, Saleem Abdulrasool <<a href="mailto:compnerd@compnerd.org" target="_blank">compnerd@compnerd.org</a>> wrote:</div><br><div><br><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div class="gmail_quote" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">On Fri, Oct 3, 2014 at 5:18 PM, Steven Wu<span> </span><span dir="ltr"><<a href="mailto:stevenwu@apple.com" target="_blank">stevenwu@apple.com</a>></span><span> </span>wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Author: steven_wu<br>Date: Fri Oct 3 19:18:59 2014<br>New Revision: 219040<br><br>URL:<span> </span><a href="http://llvm.org/viewvc/llvm-project?rev=219040&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=219040&view=rev</a><br>Log:<br>Fix the armv7 thumb builtins on darwin<br><br>The arm builtins converted into thumb in r213481 are not working<br>on darwin. On apple platforms, .thumb_func directive is required<br>to generated correct symbols for thumb functions.<br><br><<a>rdar://problem/18523605</a>><br><br>Modified:<br> <span> </span>compiler-rt/trunk/lib/builtins/arm/bswapdi2.S<br> <span> </span>compiler-rt/trunk/lib/builtins/arm/bswapsi2.S<br> <span> </span>compiler-rt/trunk/lib/builtins/arm/clzdi2.S<br> <span> </span>compiler-rt/trunk/lib/builtins/arm/clzsi2.S<br> <span> </span>compiler-rt/trunk/lib/builtins/arm/divmodsi4.S<br> <span> </span>compiler-rt/trunk/lib/builtins/arm/divsi3.S<br> <span> </span>compiler-rt/trunk/lib/builtins/arm/modsi3.S<br> <span> </span>compiler-rt/trunk/lib/builtins/arm/sync-ops.h<br> <span> </span>compiler-rt/trunk/lib/builtins/arm/udivmodsi4.S<br> <span> </span>compiler-rt/trunk/lib/builtins/arm/udivsi3.S<br> <span> </span>compiler-rt/trunk/lib/builtins/arm/umodsi3.S<br> <span> </span>compiler-rt/trunk/lib/builtins/assembly.h<br></blockquote><div><br></div><div>Sorry to resurrect this thread again, but, this makes the code harder to follow IMO. You are now using DEFINE_COMPILERRT_THUMB_FUNCTION to declare a function that may actually be an ARM function by sinking a similar check to the .thumb into the header.</div><div><br></div><div>What happens if someone uses a different check for the .thumb and then uses the THUMB_FUNCTION macro? Or doesn't use it? I think that it might be better to hoist the .thumb_func into the implementation so that it is more immediately obvious.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">Modified: compiler-rt/trunk/lib/builtins/arm/bswapdi2.S<br>URL:<span> </span><a href="http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/builtins/arm/bswapdi2.S?rev=219040&r1=219039&r2=219040&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/builtins/arm/bswapdi2.S?rev=219040&r1=219039&r2=219040&view=diff</a><br>==============================================================================<br>--- compiler-rt/trunk/lib/builtins/arm/bswapdi2.S (original)<br>+++ compiler-rt/trunk/lib/builtins/arm/bswapdi2.S Fri Oct 3 19:18:59 2014<br>@@ -21,7 +21,7 @@<br> // Reverse all the bytes in a 64-bit integer.<br> //<br> <span> </span>.p2align 2<br>-DEFINE_COMPILERRT_FUNCTION(__bswapdi2)<br>+DEFINE_COMPILERRT_THUMB_FUNCTION(__bswapdi2)<br> #if __ARM_ARCH < 6<br> // before armv6 does not have "rev" instruction<br> // r2 = rev(r0)<br><br>Modified: compiler-rt/trunk/lib/builtins/arm/bswapsi2.S<br>URL:<span> </span><a href="http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/builtins/arm/bswapsi2.S?rev=219040&r1=219039&r2=219040&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/builtins/arm/bswapsi2.S?rev=219040&r1=219039&r2=219040&view=diff</a><br>==============================================================================<br>--- compiler-rt/trunk/lib/builtins/arm/bswapsi2.S (original)<br>+++ compiler-rt/trunk/lib/builtins/arm/bswapsi2.S Fri Oct 3 19:18:59 2014<br>@@ -21,7 +21,7 @@<br> // Reverse all the bytes in a 32-bit integer.<br> //<br> <span> </span>.p2align 2<br>-DEFINE_COMPILERRT_FUNCTION(__bswapsi2)<br>+DEFINE_COMPILERRT_THUMB_FUNCTION(__bswapsi2)<br> #if __ARM_ARCH < 6<br> // before armv6 does not have "rev" instruction<br> <span> </span>eor r1, r0, r0, ror #16<br><br>Modified: compiler-rt/trunk/lib/builtins/arm/clzdi2.S<br>URL:<span> </span><a href="http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/builtins/arm/clzdi2.S?rev=219040&r1=219039&r2=219040&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/builtins/arm/clzdi2.S?rev=219040&r1=219039&r2=219040&view=diff</a><br>==============================================================================<br>--- compiler-rt/trunk/lib/builtins/arm/clzdi2.S (original)<br>+++ compiler-rt/trunk/lib/builtins/arm/clzdi2.S Fri Oct 3 19:18:59 2014<br>@@ -21,7 +21,7 @@<br><br><br> <span> </span>.p2align 2<br>-DEFINE_COMPILERRT_FUNCTION(__clzdi2)<br>+DEFINE_COMPILERRT_THUMB_FUNCTION(__clzdi2)<br> #ifdef __ARM_FEATURE_CLZ<br> #ifdef __ARMEB__<br> <span> </span>cmp r0, 0<br><br>Modified: compiler-rt/trunk/lib/builtins/arm/clzsi2.S<br>URL:<span> </span><a href="http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/builtins/arm/clzsi2.S?rev=219040&r1=219039&r2=219040&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/builtins/arm/clzsi2.S?rev=219040&r1=219039&r2=219040&view=diff</a><br>==============================================================================<br>--- compiler-rt/trunk/lib/builtins/arm/clzsi2.S (original)<br>+++ compiler-rt/trunk/lib/builtins/arm/clzsi2.S Fri Oct 3 19:18:59 2014<br>@@ -20,7 +20,7 @@<br> #endif<br><br> <span> </span>.p2align 2<br>-DEFINE_COMPILERRT_FUNCTION(__clzsi2)<br>+DEFINE_COMPILERRT_THUMB_FUNCTION(__clzsi2)<br> #ifdef __ARM_FEATURE_CLZ<br> <span> </span>clz r0, r0<br> <span> </span>JMP(lr)<br><br>Modified: compiler-rt/trunk/lib/builtins/arm/divmodsi4.S<br>URL:<span> </span><a href="http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/builtins/arm/divmodsi4.S?rev=219040&r1=219039&r2=219040&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/builtins/arm/divmodsi4.S?rev=219040&r1=219039&r2=219040&view=diff</a><br>==============================================================================<br>--- compiler-rt/trunk/lib/builtins/arm/divmodsi4.S (original)<br>+++ compiler-rt/trunk/lib/builtins/arm/divmodsi4.S Fri Oct 3 19:18:59 2014<br>@@ -32,7 +32,7 @@<br> @ value is the quotient, the remainder is placed in the variable.<br><br> <span> </span>.p2align 3<br>-DEFINE_COMPILERRT_FUNCTION(__divmodsi4)<br>+DEFINE_COMPILERRT_THUMB_FUNCTION(__divmodsi4)<br> #if __ARM_ARCH_EXT_IDIV__<br> <span> </span>tst r1, r1<br> <span> </span>beq LOCAL_LABEL(divzero)<br><br>Modified: compiler-rt/trunk/lib/builtins/arm/divsi3.S<br>URL:<span> </span><a href="http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/builtins/arm/divsi3.S?rev=219040&r1=219039&r2=219040&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/builtins/arm/divsi3.S?rev=219040&r1=219039&r2=219040&view=diff</a><br>==============================================================================<br>--- compiler-rt/trunk/lib/builtins/arm/divsi3.S (original)<br>+++ compiler-rt/trunk/lib/builtins/arm/divsi3.S Fri Oct 3 19:18:59 2014<br>@@ -33,7 +33,7 @@ DEFINE_AEABI_FUNCTION_ALIAS(__aeabi_idiv<br> @ int __divsi3(int divident, int divisor)<br> @ Calculate and return the quotient of the (signed) division.<br><br>-DEFINE_COMPILERRT_FUNCTION(__divsi3)<br>+DEFINE_COMPILERRT_THUMB_FUNCTION(__divsi3)<br> #if __ARM_ARCH_EXT_IDIV__<br> <span> </span>tst r1,r1<br> <span> </span>beq LOCAL_LABEL(divzero)<br><br>Modified: compiler-rt/trunk/lib/builtins/arm/modsi3.S<br>URL:<span> </span><a href="http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/builtins/arm/modsi3.S?rev=219040&r1=219039&r2=219040&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/builtins/arm/modsi3.S?rev=219040&r1=219039&r2=219040&view=diff</a><br>==============================================================================<br>--- compiler-rt/trunk/lib/builtins/arm/modsi3.S (original)<br>+++ compiler-rt/trunk/lib/builtins/arm/modsi3.S Fri Oct 3 19:18:59 2014<br>@@ -30,7 +30,7 @@<br> @ Calculate and return the remainder of the (signed) division.<br><br> <span> </span>.p2align 3<br>-DEFINE_COMPILERRT_FUNCTION(__modsi3)<br>+DEFINE_COMPILERRT_THUMB_FUNCTION(__modsi3)<br> #if __ARM_ARCH_EXT_IDIV__<br> <span> </span>tst r1, r1<br> <span> </span>beq LOCAL_LABEL(divzero)<br><br>Modified: compiler-rt/trunk/lib/builtins/arm/sync-ops.h<br>URL:<span> </span><a href="http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/builtins/arm/sync-ops.h?rev=219040&r1=219039&r2=219040&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/builtins/arm/sync-ops.h?rev=219040&r1=219039&r2=219040&view=diff</a><br>==============================================================================<br>--- compiler-rt/trunk/lib/builtins/arm/sync-ops.h (original)<br>+++ compiler-rt/trunk/lib/builtins/arm/sync-ops.h Fri Oct 3 19:18:59 2014<br>@@ -19,7 +19,7 @@<br> .p2align 2 ; \<br> .thumb ; \<br> .syntax unified ; \<br>- DEFINE_COMPILERRT_FUNCTION(__sync_fetch_and_ ## op) \<br>+ DEFINE_COMPILERRT_THUMB_FUNCTION(__sync_fetch_and_ ## op) \<br> dmb ; \<br> mov r12, r0 ; \<br> LOCAL_LABEL(tryatomic_ ## op): \<br>@@ -35,7 +35,7 @@<br> .p2align 2 ; \<br> .thumb ; \<br> .syntax unified ; \<br>- DEFINE_COMPILERRT_FUNCTION(__sync_fetch_and_ ## op) \<br>+ DEFINE_COMPILERRT_THUMB_FUNCTION(__sync_fetch_and_ ## op) \<br> push {r4, r5, r6, lr} ; \<br> dmb ; \<br> mov r12, r0 ; \<br><br>Modified: compiler-rt/trunk/lib/builtins/arm/udivmodsi4.S<br>URL:<span> </span><a href="http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/builtins/arm/udivmodsi4.S?rev=219040&r1=219039&r2=219040&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/builtins/arm/udivmodsi4.S?rev=219040&r1=219039&r2=219040&view=diff</a><br>==============================================================================<br>--- compiler-rt/trunk/lib/builtins/arm/udivmodsi4.S (original)<br>+++ compiler-rt/trunk/lib/builtins/arm/udivmodsi4.S Fri Oct 3 19:18:59 2014<br>@@ -27,7 +27,7 @@<br> @ value is the quotient, the remainder is placed in the variable.<br><br> <span> </span>.p2align 2<br>-DEFINE_COMPILERRT_FUNCTION(__udivmodsi4)<br>+DEFINE_COMPILERRT_THUMB_FUNCTION(__udivmodsi4)<br> #if __ARM_ARCH_EXT_IDIV__<br> <span> </span>tst r1, r1<br> <span> </span>beq LOCAL_LABEL(divby0)<br><br>Modified: compiler-rt/trunk/lib/builtins/arm/udivsi3.S<br>URL:<span> </span><a href="http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/builtins/arm/udivsi3.S?rev=219040&r1=219039&r2=219040&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/builtins/arm/udivsi3.S?rev=219040&r1=219039&r2=219040&view=diff</a><br>==============================================================================<br>--- compiler-rt/trunk/lib/builtins/arm/udivsi3.S (original)<br>+++ compiler-rt/trunk/lib/builtins/arm/udivsi3.S Fri Oct 3 19:18:59 2014<br>@@ -27,7 +27,7 @@ DEFINE_AEABI_FUNCTION_ALIAS(__aeabi_uidi<br> @ unsigned int __udivsi3(unsigned int divident, unsigned int divisor)<br> @ Calculate and return the quotient of the (unsigned) division.<br><br>-DEFINE_COMPILERRT_FUNCTION(__udivsi3)<br>+DEFINE_COMPILERRT_THUMB_FUNCTION(__udivsi3)<br> #if __ARM_ARCH_EXT_IDIV__<br> <span> </span>tst r1, r1<br> <span> </span>beq LOCAL_LABEL(divby0)<br><br>Modified: compiler-rt/trunk/lib/builtins/arm/umodsi3.S<br>URL:<span> </span><a href="http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/builtins/arm/umodsi3.S?rev=219040&r1=219039&r2=219040&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/builtins/arm/umodsi3.S?rev=219040&r1=219039&r2=219040&view=diff</a><br>==============================================================================<br>--- compiler-rt/trunk/lib/builtins/arm/umodsi3.S (original)<br>+++ compiler-rt/trunk/lib/builtins/arm/umodsi3.S Fri Oct 3 19:18:59 2014<br>@@ -24,7 +24,7 @@<br> @ Calculate and return the remainder of the (unsigned) division.<br><br> <span> </span>.p2align 2<br>-DEFINE_COMPILERRT_FUNCTION(__umodsi3)<br>+DEFINE_COMPILERRT_THUMB_FUNCTION(__umodsi3)<br> #if __ARM_ARCH_EXT_IDIV__<br> <span> </span>tst r1, r1<br> <span> </span>beq LOCAL_LABEL(divby0)<br><br>Modified: compiler-rt/trunk/lib/builtins/assembly.h<br>URL:<span> </span><a href="http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/builtins/assembly.h?rev=219040&r1=219039&r2=219040&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/compiler-rt/trunk/lib/builtins/assembly.h?rev=219040&r1=219039&r2=219040&view=diff</a><br>==============================================================================<br>--- compiler-rt/trunk/lib/builtins/assembly.h (original)<br>+++ compiler-rt/trunk/lib/builtins/assembly.h Fri Oct 3 19:18:59 2014<br>@@ -28,6 +28,10 @@<br> // tell linker it can break up file at label boundaries<br> #define FILE_LEVEL_DIRECTIVE .subsections_via_symbols<br> #define SYMBOL_IS_FUNC(name)<br>+// <a>rdar://problem/18523605</a><br>+#if __ARM_ARCH_ISA_THUMB == 2<br>+#define THUMB_FUNC .thumb_func<br>+#endif<br> #elif defined(__ELF__)<br> #define HIDDEN(name) .hidden name<br> #define LOCAL_LABEL(name) .L_##name<br>@@ -97,6 +101,14 @@<br> SYMBOL_IS_FUNC(SYMBOL_NAME(name)) SEPARATOR \<br> DECLARE_SYMBOL_VISIBILITY(name) \<br> SYMBOL_NAME(name):<br>+<br>+#define DEFINE_COMPILERRT_THUMB_FUNCTION(name) \<br>+ FILE_LEVEL_DIRECTIVE SEPARATOR \<br>+ .globl SYMBOL_NAME(name) SEPARATOR \<br>+ THUMB_FUNC \<br>+ SYMBOL_IS_FUNC(SYMBOL_NAME(name)) SEPARATOR \<br>+ DECLARE_SYMBOL_VISIBILITY(name) \<br>+ SYMBOL_NAME(name):<br><br> #define DEFINE_COMPILERRT_PRIVATE_FUNCTION(name) \<br> FILE_LEVEL_DIRECTIVE SEPARATOR \<br><br><br>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br></blockquote></div><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><br clear="all" style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><br></div><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">--<span> </span></span><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">Saleem Abdulrasool</span><br style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><span style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px;float:none;display:inline!important">compnerd (at) compnerd (dot) org</span></div></blockquote></div><br></div></div></div></div></div></blockquote></div><br><br clear="all"><div><br></div>-- <br>Saleem Abdulrasool<br>compnerd (at) compnerd (dot) org
</div></div>