[builtins] Fix c?zdi2 on sparc64/Linux and ignore riscv32

[jrtc27]

Summary

On sparc64/Linux, sparc64 isn't defined; the canonical way of
checking for sparc64 is sparc && arch64, which also works on the
BSDs and Solaris. Since this problem does not occur on 32-bit
architectures, riscv32 can be ignored. This fixes and refines rL324593.

[jrtc27]

Actually this might be ok on sparc32? I assume __builtin_ctz will instead resolve to _ctzsi2 being a 32-bit architecture? In which case, shouldn't the __riscv__ one be checking __SIZEOF_POINTER__ == 8?

[JDevlieghere]

In D43146#1003907, @jrtc27 wrote:

Actually this might be ok on sparc32? I assume __builtin_ctz will instead resolve to _ctzsi2 being a 32-bit architecture? In which case, shouldn't the __riscv__ one be checking __SIZEOF_POINTER__ == 8?

Feel free to commandeer this revision with what you think should be the proper solution. As Hans mentioned in https://llvm.org/PR36318 we should do it soon if we want to have this in the 6.0 release.

[jrtc27]

[builtins] Fix c?zdi2 on sparc64/Linux and refine mips64/riscv tests

On sparc64/Linux, sparc64 isn't defined; the canonical way of
checking for sparc64 is sparc && arch64, which also works on the
BSDs and Solaris. Newer mips64 CPUs have a clz instruction which is used
for both clz and ctz (except in mips16 mode), so those do not need the
workaround, nor does riscv32. This fixes and refines rL324593.

[joerg]

We tried to keep the condition simple. I.e. does the compiler on any of those platforms ever use the libcall? If not, it is IMO not worth the complexity.

[jrtc27]

In D43146#1012323, @joerg wrote:

We tried to keep the condition simple. I.e. does the compiler on any of those platforms ever use the libcall? If not, it is IMO not worth the complexity.

Ok, I can see the argument for getting rid of the mips64 complexity, but I'd say the __SIZEOF_POINTER__ check should stay for riscv, since it definitely won't be a problem for riscv32. Would you agree with that?

[jrtc27]

Dropped complicated logic for mips64

[kito-cheng]

lib/builtins/clzdi2.c

22 ↗

(On Diff #134996)

RISC-V define __riscv instead of __riscv__.

[jrtc27]

Updated to check __riscv instead of the deprecated __riscv__

[kito-cheng]

LGTM for RISC-V part :)

[compnerd]

Please clang-format the conditional (we keep the operator on the previous line).

I'm wondering if this is becoming a pretty complex set of cases and it's better to just do __SIZEOF_POINTER__ == 8. The question is, how does it impact x86_64 and aarch64, and if those are not impacted in a meaningful way, I think that we should just go ahead and simplify the condition to all 64-bit targets. Then again, this is a GCC specific codepath, and I'm tempted to say, we can afford the overhead of an additional call.

[jrtc27]

In D43146#1013976, @compnerd wrote:

Please clang-format the conditional (we keep the operator on the previous line).

I'm wondering if this is becoming a pretty complex set of cases and it's better to just do __SIZEOF_POINTER__ == 8. The question is, how does it impact x86_64 and aarch64, and if those are not impacted in a meaningful way, I think that we should just go ahead and simplify the condition to all 64-bit targets. Then again, this is a GCC specific codepath, and I'm tempted to say, we can afford the overhead of an additional call.

I guess for 64-bit architectures with a native clz instruction we would expect that instruction to be used for the DI version too, and therefore not be calling __clzdi2 in the first place? I.e. the only 64-bit platforms which actually call __clzdi2 are the ones where we have a problem? I could be wrong about this though.

[glaubitz]

Is there any chance this PR can get completed?

[jrtc27]

Fixed formatting with the help of clang-format.

[jrtc27]

Ping?

[JDevlieghere]

This LGTM. @compnerd can you live with this?

[compnerd]

Sure, I think I can live with this.

[glaubitz]

@JDevlieghere Can you commit this fix on behalf of jrtc27? He's currently on vacation.

[glaubitz]

Can someone commit this, please?

[kristina]

Landing it per request as it's approved (just going to be a second, running it on a buildbot first).

compiler-rt/trunk/lib/builtins/clzdi2.c

  *
  * ===----------------------------------------------------------------------===
  */
 
 #include "int_lib.h"
 
 /* Returns: the number of leading 0-bits */
 
-#if !defined(__clang__) && (defined(__sparc64__) || defined(__mips64) || defined(__riscv__))
-/* gcc resolves __builtin_clz -> __clzdi2 leading to infinite recursion */
+#if !defined(__clang__) &&                                                     \
+    ((defined(__sparc__) && defined(__arch64__)) ||                            \
+     defined(__mips64) ||                                                      \
+     (defined(__riscv) && __SIZEOF_POINTER__ >= 8))
+/* On 64-bit architectures with neither a native clz instruction nor a native
+ * ctz instruction, gcc resolves __builtin_clz to __clzdi2 rather than
+ * __clzsi2, leading to infinite recursion. */
 #define __builtin_clz(a) __clzsi2(a)
 extern si_int __clzsi2(si_int);
 #endif
 
 /* Precondition: a != 0 */
 
 COMPILER_RT_ABI si_int
 __clzdi2(di_int a)
 {
     dwords x;
     x.all = a;
     const si_int f = -(x.s.high == 0);
     return __builtin_clz((x.s.high & ~f) | (x.s.low & f)) +
            (f & ((si_int)(sizeof(si_int) * CHAR_BIT)));
 }

compiler-rt/trunk/lib/builtins/ctzdi2.c

  *
  * ===----------------------------------------------------------------------===
  */
 
 #include "int_lib.h"
 
 /* Returns: the number of trailing 0-bits  */
 
-#if !defined(__clang__) && (defined(__sparc64__) || defined(__mips64) || defined(__riscv__))
-/* gcc resolves __builtin_ctz -> __ctzdi2 leading to infinite recursion */
+#if !defined(__clang__) &&                                                     \
+    ((defined(__sparc__) && defined(__arch64__)) ||                            \
+     defined(__mips64) ||                                                      \
+     (defined(__riscv) && __SIZEOF_POINTER__ >= 8))
+/* On 64-bit architectures with neither a native clz instruction nor a native
+ * ctz instruction, gcc resolves __builtin_ctz to __ctzdi2 rather than
+ * __ctzsi2, leading to infinite recursion. */
 #define __builtin_ctz(a) __ctzsi2(a)
 extern si_int __ctzsi2(si_int);
 #endif
 
 /* Precondition: a != 0 */
 
 COMPILER_RT_ABI si_int
 __ctzdi2(di_int a)
 {
     dwords x;
     x.all = a;
     const si_int f = -(x.s.low == 0);
     return __builtin_ctz((x.s.high & f) | (x.s.low & ~f)) +
               (f & ((si_int)(sizeof(si_int) * CHAR_BIT)));
 }