[rdma-core,2/2] mlx5: Convert explicitly to signed char

Commit Message

Yishai Hadas Aug. 30, 2017, 2:19 p.m. UTC

From: Leon Romanovsky <[e-mail]>

The 0x80 value is a canonical way to mark as not-used in intrinsics
function calls.

However the gcc compiler is not aware of this convention and compilation
with -Werror=overflow option will cause to compilation failure, because
_mm_set_epi8() call expects chars as an input.

In order to avoid it, we will convert 0x80 to be -128.

Fixes: 00c91653ef9a ("mlx5: Add WQE segments implementation")
Signed-off-by: Leon Romanovsky <[e-mail]>
Reviewed-by: Yishai Hadas <[e-mail]>
---
 providers/mlx5/mlx5dv.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Jason Gunthorpe Aug. 30, 2017, 4:49 p.m. UTC | #1

On Wed, Aug 30, 2017 at 05:19:24PM +0300, Yishai Hadas wrote:
> From: Leon Romanovsky <[e-mail]>
> 
> The 0x80 value is a canonical way to mark as not-used in intrinsics
> function calls.

This commit message is not good, try:

_mm_shuffle_epi8 requires 0x80 to set the output byte to zero, but
_mm_set_epi8() accepts char. If gcc is compiling in a configuration
with a signed char then it can produce a -Werror=overflow warning.

And this fix is wrong, since it just moves the warning to
configurations that have an unsigned char. (eg -funsigned-char)

It is really broken that the _mm_set_epi uses a char as input :|

I think you need to do this instead:

#include <limits.h>
#if CHAR_MIN < 0
#define SHUFFLE_IGNORE  -128
#else
#define SHUFFLE_IGNORE  0x80
#endif

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Leon Romanovsky Aug. 30, 2017, 5:44 p.m. UTC | #2

On Wed, Aug 30, 2017 at 10:49:52AM -0600, Jason Gunthorpe wrote:
> On Wed, Aug 30, 2017 at 05:19:24PM +0300, Yishai Hadas wrote:
> > From: Leon Romanovsky <[e-mail]>
> >
> > The 0x80 value is a canonical way to mark as not-used in intrinsics
> > function calls.
>
> This commit message is not good, try:
>
> _mm_shuffle_epi8 requires 0x80 to set the output byte to zero, but
> _mm_set_epi8() accepts char. If gcc is compiling in a configuration
> with a signed char then it can produce a -Werror=overflow warning.
>
> And this fix is wrong, since it just moves the warning to
> configurations that have an unsigned char. (eg -funsigned-char)

I have gut feeling that with such option "-funsigned-char" half of the
world will break.

>
> It is really broken that the _mm_set_epi uses a char as input :|
>
> I think you need to do this instead:
>
> #include <limits.h>
> #if CHAR_MIN < 0
> #define SHUFFLE_IGNORE  -128
> #else
> #define SHUFFLE_IGNORE  0x80
> #endif
>
> Jason
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Jason Gunthorpe Aug. 30, 2017, 5:49 p.m. UTC | #3

On Wed, Aug 30, 2017 at 08:44:02PM +0300, Leon Romanovsky wrote:
> On Wed, Aug 30, 2017 at 10:49:52AM -0600, Jason Gunthorpe wrote:
> > On Wed, Aug 30, 2017 at 05:19:24PM +0300, Yishai Hadas wrote:
> > > From: Leon Romanovsky <[e-mail]>
> > >
> > > The 0x80 value is a canonical way to mark as not-used in intrinsics
> > > function calls.
> >
> > This commit message is not good, try:
> >
> > _mm_shuffle_epi8 requires 0x80 to set the output byte to zero, but
> > _mm_set_epi8() accepts char. If gcc is compiling in a configuration
> > with a signed char then it can produce a -Werror=overflow warning.
> >
> > And this fix is wrong, since it just moves the warning to
> > configurations that have an unsigned char. (eg -funsigned-char)
> 
> I have gut feeling that with such option "-funsigned-char" half of the
> world will break.

Nope, some arches default to it on. aarch64 for instance.

This is why we have uint8_t, and using 'char' for anything other than
a string character is very wrong.

Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/providers/mlx5/mlx5dv.h b/providers/mlx5/mlx5dv.h
index 5d11625..e9efcf5 100644
--- a/providers/mlx5/mlx5dv.h
+++ b/providers/mlx5/mlx5dv.h
@@ -499,7 +499,7 @@  void mlx5dv_x86_set_ctrl_seg(struct mlx5_wqe_ctrl_seg *seg, uint16_t pi,
 				     (signature << 24) | (opcode << 16) | (opmod << 8) | fm_ce_se);
 	__m128i mask = _mm_set_epi8(15, 14, 13, 12,	/* immediate */
 				     0,			/* signal/fence_mode */
-				     0x80, 0x80,	/* reserved */
+				     -128, -128,	/* reserved */
 				     3,			/* signature */
 				     6,			/* data size */
 				     8, 9, 10,		/* QP num */