Re: [PATCH] rs6000, Fix header comment for intrinsic function

2020-04-29 Thread Bill Schmidt via Gcc-patches 

On 4/22/20 1:20 PM, Carl Love wrote:

GCC maintainers:

The following is a trivial patch to fix a comment describing the
intrinsic function _mm_movemask_epi8.  The comment was expanded to
clarify the layout of the returned result.

The patch does not make any functional changes.

Please let me know if the patch is OK for mainline and backporting as
appropriate.

Thanks.

  Carl Love
---
rs6000, Fix header comment for intrinsic function _mm_movemask_epi8

gcc/ChangeLog

2020-04-22  Carl Love  

* config/rs6000/emmintrin.h (_mm_movemask_epi8): Fix comment for the
function.


Drop "for the function" as Will suggested.



Signed-off-by: Carl Love 
---
  gcc/config/rs6000/emmintrin.h | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/gcc/config/rs6000/emmintrin.h b/gcc/config/rs6000/emmintrin.h
index 2462cf5bdac..0872a75c0de 100644
--- a/gcc/config/rs6000/emmintrin.h
+++ b/gcc/config/rs6000/emmintrin.h
@@ -2033,7 +2033,9 @@ _mm_min_epu8 (__m128i __A, __m128i __B)
  #ifdef _ARCH_PWR8
  /* Intrinsic functions that require PowerISA 2.07 minimum.  */

-/* Creates a 4-bit mask from the most significant bits of the SPFP values.  */
+/* Creates a 16-bit mask from the most significant bits of the sixteen 8-bit
+   values.  The 16-bit result is placed in bits[48:63], bits [0:47] and
+   bits [64:127] are  set to zero.  */


The description of the emulated function is

"Create mask from the most significant bit of each 8-bit element in a, and store the 
result in dst." [*]

Therefore I suggest you change the comment to

"Return a mask created from the most significant bit of each 8-bit element in 
A."

OK to commit as obvious with these changes.

Thanks,
Bill

[*] 
https://software.intel.com/sites/landingpage/IntrinsicsGuide/#text=_mm_movemask_ep=3864


  extern __inline int __attribute__((__gnu_inline__, __always_inline__, 
__artificial__))
  _mm_movemask_epi8 (__m128i __A)
  {

Re: [PATCH] rs6000, Fix header comment for intrinsic function

2020-04-23 Thread will schmidt via Gcc-patches 
On Wed, 2020-04-22 at 11:20 -0700, Carl Love wrote:
> GCC maintainers:
> 

Hi,


> The following is a trivial patch to fix a comment describing the
> intrinsic function _mm_movemask_epi8.  The comment was expanded to
> clarify the layout of the returned result. 

Something seems wrong there, see below.

> 
> The patch does not make any functional changes.
> 
> Please let me know if the patch is OK for mainline and backporting as
> appropriate.
> 
> Thanks.
> 
>  Carl Love
> ---
> rs6000, Fix header comment for intrinsic function _mm_movemask_epi8
> 
> gcc/ChangeLog
> 
> 2020-04-22  Carl Love  
> 
>   * config/rs6000/emmintrin.h (_mm_movemask_epi8): Fix comment
> for the
>   function.


drop /for the function/

> 
> Signed-off-by: Carl Love 
> ---
>  gcc/config/rs6000/emmintrin.h | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/gcc/config/rs6000/emmintrin.h
> b/gcc/config/rs6000/emmintrin.h
> index 2462cf5bdac..0872a75c0de 100644
> --- a/gcc/config/rs6000/emmintrin.h
> +++ b/gcc/config/rs6000/emmintrin.h
> @@ -2033,7 +2033,9 @@ _mm_min_epu8 (__m128i __A, __m128i __B)
>  #ifdef _ARCH_PWR8
>  /* Intrinsic functions that require PowerISA 2.07 minimum.  */
> 
> -/* Creates a 4-bit mask from the most significant bits of the SPFP
> values.  */
> +/* Creates a 16-bit mask from the most significant bits of the
> sixteen 8-bit
> +   values.  The 16-bit result is placed in bits[48:63], bits [0:47]
> and
> +   bits [64:127] are  set to zero.  */


That function returns an int out of one of the elements of the result
vector.


extern __inline int __attribute__((__gnu_inline__, __always_inline__,
__artificial__))
_mm_movemask_epi8 (__m128i __A)
{
...
  __vector unsigned long long result;
...

So the description of the intermediate __vector unsigned long long
variable, if actually needed, would fit better near the assignment to
result.

  result = ((__vector unsigned long long)
vec_vbpermq ((__vector unsigned char) __A,
 (__vector unsigned char) perm_mask));

But then you would probably want to clarify that the return is only one
of those long long vector elements. 

#ifdef __LITTLE_ENDIAN__
  return result[1];
#else
  return result[0];
#end
if


thanks,
-Will

>  extern __inline int __attribute__((__gnu_inline__,
> __always_inline__, __artificial__))
>  _mm_movemask_epi8 (__m128i __A)
>  {

[PATCH] rs6000, Fix header comment for intrinsic function

2020-04-22 Thread Carl Love via Gcc-patches 


GCC maintainers:

The following is a trivial patch to fix a comment describing the
intrinsic function _mm_movemask_epi8.  The comment was expanded to
clarify the layout of the returned result. 

The patch does not make any functional changes.

Please let me know if the patch is OK for mainline and backporting as
appropriate.

Thanks.

 Carl Love
---
rs6000, Fix header comment for intrinsic function _mm_movemask_epi8

gcc/ChangeLog

2020-04-22  Carl Love  

* config/rs6000/emmintrin.h (_mm_movemask_epi8): Fix comment for the
function.

Signed-off-by: Carl Love 
---
 gcc/config/rs6000/emmintrin.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/gcc/config/rs6000/emmintrin.h b/gcc/config/rs6000/emmintrin.h
index 2462cf5bdac..0872a75c0de 100644
--- a/gcc/config/rs6000/emmintrin.h
+++ b/gcc/config/rs6000/emmintrin.h
@@ -2033,7 +2033,9 @@ _mm_min_epu8 (__m128i __A, __m128i __B)
 #ifdef _ARCH_PWR8
 /* Intrinsic functions that require PowerISA 2.07 minimum.  */
 
-/* Creates a 4-bit mask from the most significant bits of the SPFP values.  */
+/* Creates a 16-bit mask from the most significant bits of the sixteen 8-bit
+   values.  The 16-bit result is placed in bits[48:63], bits [0:47] and
+   bits [64:127] are  set to zero.  */
 extern __inline int __attribute__((__gnu_inline__, __always_inline__, 
__artificial__))
 _mm_movemask_epi8 (__m128i __A)
 {
-- 
2.17.1