Re: [PATCH] Fix PR64718: bad 16-bit bswap replacement

[Richard Biener <rguenther at suse dot de>]

On Fri, 23 Jan 2015, Thomas Preud'homme wrote:

> Hi Richard,
> 
> When detecting a 16-bit hand-crafted byte swap, the bswap pass replaces the sequence of statements by a rotation left of 8 bits of the same type as the source. For source whose type size is bigger than 16 bits this is wrong. For instance,
> 
> int swap(int x)
> {
> return (unsigned short) ((unsigned short) x << 8 | (unsigned short) x >> 8);
> }
> 
> is optimized incorrectly. This patch select the type of the rotation to be 16 bits in size and with the same sign as the source, doing a conversation if necessary before doing the rotation.
> 
> ChangeLog entries are as follows:
> 
> *** gcc/ChangeLog ***
> 
> 2015-01-22  Thomas Preud'homme  <thomas.preudhomme@arm.com>
> 
>     PR tree-optimization/64718
>     * tree-ssa-math-opts.c (bswap_replace): Make bswap_type be a short
>     type of same sign as src and convert src to that type if necessary for
>     all bswap sizes. Fix rotation right notation in nearby comment.
> 
> 
> *** gcc/testsuite/ChangeLog ***
> 
> 2015-01-22  Thomas Preud'homme  <thomas.preudhomme@arm.com>
> 
>     PR tree-optimization/64718
>     * gcc.c-torture/execute/pr64718.c: New test.
> 
> 
> diff --git a/gcc/testsuite/gcc.c-torture/execute/pr64718.c b/gcc/testsuite/gcc.c-torture/execute/pr64718.c
> new file mode 100644
> index 0000000..58773e0
> --- /dev/null
> +++ b/gcc/testsuite/gcc.c-torture/execute/pr64718.c
> @@ -0,0 +1,18 @@
> +static int __attribute__ ((noinline, noclone))
> +swap (int x)
> +{
> +  return (unsigned short) ((unsigned short) x << 8 | (unsigned short) x >> 8);
> +}
> +
> +static int a = 0x1234;
> +
> +int
> +main (void)
> +{
> +  int b = 0x1234;
> +  if (swap (a) != 0x3412)
> +    __builtin_abort ();
> +  if (swap (b) != 0x3412)
> +    __builtin_abort ();
> +  return 0;
> +}
> diff --git a/gcc/tree-ssa-math-opts.c b/gcc/tree-ssa-math-opts.c
> index 479f49f..0ff8040 100644
> --- a/gcc/tree-ssa-math-opts.c
> +++ b/gcc/tree-ssa-math-opts.c
> @@ -2355,30 +2355,32 @@ bswap_replace (gimple cur_stmt, gimple src_stmt, tree fndecl, tree bswap_type,
>  
>    tmp = src;
>  
> +  if (bswap && n->range == 16)
> +    bswap_type = TYPE_UNSIGNED (TREE_TYPE (src)) ? short_unsigned_type_node
> +						 : short_integer_type_node;

I don't think using short_unsigned_type_node or short_integer_type_node
is correct - that depends on the SHORT_TYPE_SIZE target macro which
may very well not match HImode.

I think for the rotation itself whether the type is signed or unsigned 
doesn't matter and the bswap builtins expect unsigned input.  So I think 
you should use an unsigned type unconditionally.

Which means you can simply use build_nonstandard_integer_type (16, 1);
or even bswap_type as-is (it should match the unsigned builtin argument
type already?)

Ok with that change.

Thanks,
Richard.

> +
> +  /* Convert the src expression if necessary.  */
> +  if (!useless_type_conversion_p (TREE_TYPE (tmp), bswap_type))
> +    {
> +      gimple convert_stmt;
> +
> +      tmp = make_temp_ssa_name (bswap_type, NULL, "bswapsrc");
> +      convert_stmt = gimple_build_assign (tmp, NOP_EXPR, src);
> +      gsi_insert_before (&gsi, convert_stmt, GSI_SAME_STMT);
> +    }
> +
>    /* Canonical form for 16 bit bswap is a rotate expression.  Only 16bit values
>       are considered as rotation of 2N bit values by N bits is generally not
> -     equivalent to a bswap.  Consider for instance 0x01020304 >> 16 which gives
> -     0x03040102 while a bswap for that value is 0x04030201.  */
> +     equivalent to a bswap.  Consider for instance 0x01020304 r>> 16 which
> +     gives 0x03040102 while a bswap for that value is 0x04030201.  */
>    if (bswap && n->range == 16)
>      {
>        tree count = build_int_cst (NULL, BITS_PER_UNIT);
> -      bswap_type = TREE_TYPE (src);
> -      src = fold_build2 (LROTATE_EXPR, bswap_type, src, count);
> +      src = fold_build2 (LROTATE_EXPR, bswap_type, tmp, count);
>        bswap_stmt = gimple_build_assign (NULL, src);
>      }
>    else
> -    {
> -      /* Convert the src expression if necessary.  */
> -      if (!useless_type_conversion_p (TREE_TYPE (tmp), bswap_type))
> -	{
> -	  gimple convert_stmt;
> -	  tmp = make_temp_ssa_name (bswap_type, NULL, "bswapsrc");
> -	  convert_stmt = gimple_build_assign (tmp, NOP_EXPR, src);
> -	  gsi_insert_before (&gsi, convert_stmt, GSI_SAME_STMT);
> -	}
> -
> -      bswap_stmt = gimple_build_call (fndecl, 1, tmp);
> -    }
> +    bswap_stmt = gimple_build_call (fndecl, 1, tmp);
>  
>    tmp = tgt;
>  
> @@ -2386,6 +2388,7 @@ bswap_replace (gimple cur_stmt, gimple src_stmt, tree fndecl, tree bswap_type,
>    if (!useless_type_conversion_p (TREE_TYPE (tgt), bswap_type))
>      {
>        gimple convert_stmt;
> +
>        tmp = make_temp_ssa_name (bswap_type, NULL, "bswapdst");
>        convert_stmt = gimple_build_assign (tgt, NOP_EXPR, tmp);
>        gsi_insert_after (&gsi, convert_stmt, GSI_SAME_STMT);
> 
> 
> Testing done:
> 
>  *  Bootstrapped on x86_64 and no regression were found when running the testsuite.
>   * Equally, an arm-none-eabi GCC cross-compiler was build and no regression were found when running the testsuite on QEMU emulating a Cortex-M3
> 
> Is this ok for trunk?
> 
> Best regards,
> 
> Thomas
> 
> 
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Jennifer Guild,
Dilip Upmanyu, Graham Norton HRB 21284 (AG Nuernberg)