RE: bzr commit into mysql-6.0-falcon branch (olav:2758) Bug#37622

[Vladislav Vaintroub]

Olav, that looks great, actually ok to push for me, but here are some
thoughts mostly on style.

do you really need cas_sparc, cas_sparc_pointer32 and cas_sparc_pointer64?
Is it not enough to have cas_sparc32 and cas_sparc64 (the code for cas_sparc
looks suspiciously similar to _pointer32 variation

> +inline int cas_pointer_sparc(volatile void **target, void *compare,
> +                             void *exchange) {
> +  if (sizeof(void*) == 4) {
> +    return cas_pointer_sparc32(target, compare, exchange);
> +  }
> +  else {
> +    return cas_pointer_sparc64(target, compare, exchange);
> +  }
> +}

For Falcon, "Yet Another True Bracing Style" is used, the one you've got
here reveals your  roots too obviously;)
Also one can save 4 braces here altogether, and "else" as well

Inline function for pointer is fine (hopefully compiler will not really
generate compare and jump instructions ;)), but would not that be more
natural to have

#if SUNSTUDIO_SPECIFIC_64_BIT_PREDEFINED_MACRO
#define cas_pointer_sparc cas64
#else
#define cas_pointer_sparc cas32
#endif

I do like that you have all sparc stuff  in an extra header, maybe we should
apply this everywhere else for consistency? ( that is ,create a bunch of
headers msvc-generic, gcc-x86, gcc-ppc, solaris-intrinsics, solaris-sparc
(we don't yet use gcc-builtins, but could do so it in the future )

Vlad


> -----Original Message-----
> From: Olav Sandstaa [mailto:olav@stripped]
> Sent: Friday, July 25, 2008 11:48 PM
> To: commits@stripped
> Subject: bzr commit into mysql-6.0-falcon branch (olav:2758) Bug#37622
> 
> #At file:///home/os136802/mysql/develop/repo/mysql-6.0-falcon-sparc-ss/
> 
>  2758 Olav Sandstaa	2008-07-25
>       Fix for Bug#37622 Falcon does not compile on Solaris 9 on SPARC
> using Sun Studio compiler.
> 
>       Implements Interlock operations using Sun Studio's inline
> assembly for Solaris 9 on SPARC.
> added:
>   storage/falcon/CompareAndSwapSparc.h
> modified:
>   storage/falcon/CompareAndSwapSparc.il
>   storage/falcon/Interlock.h
> 
> per-file messages:
>   storage/falcon/CompareAndSwapSparc.h
>     Include file for the interface for the compare and swap function
> implemented using Sun Studio's
>     inline assembly.
>   storage/falcon/CompareAndSwapSparc.il
>     Implemented compare and swap using SPARC assembly.
>   storage/falcon/Interlock.h
>     Added support for Solaris 9 on SPARC. The implementation is found
> in CompareAndSwapSparc.il and is implemented using Sun Studio's inline
> assembly.
> === added file 'storage/falcon/CompareAndSwapSparc.h'
> 
> === added file 'storage/falcon/CompareAndSwapSparc.h'
> --- a/storage/falcon/CompareAndSwapSparc.h	1970-01-01 00:00:00
> +0000
> +++ b/storage/falcon/CompareAndSwapSparc.h	2008-07-25 21:47:46
> +0000
> @@ -0,0 +1,43 @@
> +/* Copyright (C) 2008 MySQL AB
> +
> +   This program is free software; you can redistribute it and/or
> modify
> +   it under the terms of the GNU General Public License as published
> by
> +   the Free Software Foundation; version 2 of the License.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program; if not, write to the Free Software
> +   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-
> 1307  USA */
> +
> +#ifndef __COMPAREANDSWAPSPARC_H
> +#define __COMPAREANDSWAPSPARC_H
> +
> +#if defined(__SUNPRO_CC) && defined(__sparc)
> +
> +/* Declaration of C prototypes for code written as assembly using
> +   Sun Studio's inline templates. The implementation is found in
> +   CompareAndSwapSparc.il */
> +extern "C" int cas_sparc(volatile int *target, int compare, int
> exchange);
> +extern "C" int cas_pointer_sparc32(volatile void **target, void
> *compare,
> +                                   void *exchange);
> +extern "C" int cas_pointer_sparc64(volatile void **target, void
> *compare,
> +                                   void *exchange);
> +
> +
> +inline int cas_pointer_sparc(volatile void **target, void *compare,
> +                             void *exchange) {
> +  if (sizeof(void*) == 4) {
> +    return cas_pointer_sparc32(target, compare, exchange);
> +  }
> +  else {
> +    return cas_pointer_sparc64(target, compare, exchange);
> +  }
> +}
> +
> +#endif /* __SUNPRO_CC && __sparc */
> +
> +#endif /* __COMPAREANDSWAPSPARC_H */
> 
> === modified file 'storage/falcon/CompareAndSwapSparc.il'
> --- a/storage/falcon/CompareAndSwapSparc.il	2008-07-10 09:27:51
> +0000
> +++ b/storage/falcon/CompareAndSwapSparc.il	2008-07-25 21:47:46
> +0000
> @@ -1,12 +1,67 @@
> -
> -
> -/* Implements inline_cas_uint - empty this far */
> -.inline compareswap
> -  nop
> -.end
> -
> -
> -/* Implements inline_cas_ptr - empty this far */
> -.inline compareswapptr
> -  nop
> +/* Copyright (C) 2008 MySQL AB
> +
> +   This program is free software; you can redistribute it and/or
> modify
> +   it under the terms of the GNU General Public License as published
> by
> +   the Free Software Foundation; version 2 of the License.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program; if not, write to the Free Software
> +   Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-
> 1307  USA */
> +
> +
> +/* Implements compare and swap. The signature of the function is:
> +
> +     int cas_sparc(volatile int *target, int compare, int exchange);
> +
> +   Return value: if the swap took place, 1 will be returned,
> +                 otherwise 0 is returned.
> +*/
> +.inline cas_sparc
> +   membar #LoadStore|#StoreStore
> +   cas [%o0],%o1,%o2
> +   cmp %o1,%o2
> +   mov 1,%o0
> +   movne %icc,0,%o0
> +   membar #LoadLoad|#LoadStore
> +.end
> +
> +
> +/* Implements compare and swap for 32 bit pointers. Signature:
> +
> +     int cas_pointer_sparc32(volatile void **target, void *compare,
> +                             void *exchange);
> +
> +   Return value: if the swap took place, 1 will be returned,
> +                 otherwise 0 is returned.
> +*/
> +.inline cas_pointer_sparc32
> +   membar #LoadStore|#StoreStore
> +   cas [%o0],%o1,%o2
> +   cmp %o1,%o2
> +   mov 1,%o0
> +   movne %icc,0,%o0
> +   membar #LoadLoad|#LoadStore
> +.end
> +
> +
> +/* Implements compare and swap for 64 bit pointers. Signature:
> +
> +     int cas_pointer_sparc64(volatile void **target, void *compare,
> +                           void *exchange);
> +
> +   Return value: if the swap took place, 1 will be returned,
> +                 otherwise 0 is returned.
> +*/
> +.inline cas_pointer_sparc64
> +   membar #LoadStore|#StoreStore
> +   casx [%o0],%o1,%o2
> +   cmp %o1,%o2
> +   mov 1,%o0
> +   movne %icc,0,%o0
> +   membar #LoadLoad|#LoadStore
>  .end
> 
> === modified file 'storage/falcon/Interlock.h'
> --- a/storage/falcon/Interlock.h	2008-07-10 09:27:51 +0000
> +++ b/storage/falcon/Interlock.h	2008-07-25 21:47:46 +0000
> @@ -19,10 +19,9 @@
>  #if defined(__sparcv8) || defined(__sparcv9) || defined(__sun)
>  #include <sys/atomic.h>
> 
> -#if defined(__SunOS_5_9)
> -extern "C" int compareswap(volatile int *target, int compare, int
> exchange);
> -extern "C" char compareswapptr(volatile void **target, void *compare,
> void *exchange);
> -#endif /* __SunOS_5_9 */
> +#if defined(__SunOS_5_9) && defined(__SUNPRO_CC) &&
> defined(__sparc)
> +#include "CompareAndSwapSparc.h"
> +#endif /* __SunOS_5_9 && __SUNPRO_CC && __sparc */
> 
>  #endif
> 
> @@ -152,11 +151,8 @@
>  #if defined(__SunOS_5_10) || defined(__SunOS_5_11)
>      return (compare == atomic_cas_uint((volatile uint_t *)target,
> compare, exchange));
>  #else
> -#  error cas not defined. We need >= Solaris 10
> -	/* Not implemented yet - just an example of how to call inline
> assembly */
> -	char ret = compareswap(target, compare, exchange);
> -
> -	return ret;
> +	/* Use inline assembly for Solaris 9 */
> +	return cas_sparc(target, compare, exchange);
>  #endif
> 
>  #else
> @@ -266,11 +262,8 @@
>  #if defined(__SunOS_5_10) || defined(__SunOS_5_11)
>      return (char)(compare == atomic_cas_ptr(target, compare,
> exchange));
>  #else
> -#  error cas not defined. We need >= Solaris 10
> -	/* Not implemented yet - just an example for calling inline
> assembly */
> -	char ret = compareswapptr(target, compare, exchange);
> -
> -	return ret;
> +	/* Use inline assembly for Solaris 9 */
> +	return cas_pointer_sparc(target, compare, exchange);
>  #endif
> 
>  #else
> 
> 
> --
> MySQL Code Commits Mailing List
> For list archives: http://lists.mysql.com/commits
> To unsubscribe:
> http://lists.mysql.com/commits?unsub=1