Re: [PATCH] Fix ctz issues (PR93231)

[Jakub Jelinek <jakub@redhat.com>]

On Wed, Jan 15, 2020 at 03:50:01PM +0000, Wilco Dijkstra wrote:
> The multiply can be signed or unsigned, and the immediate can be positive or
> negative, but the shift must be unsigned indeed. I thought the match.pd
> pattern only allows unsigned shifts, but the shift operator allows signed shifts
> too, so I've added an unsigned check on the input_type.

I guess in theory if there is some multiplication constant that for all the
power of twos + 0 would give a positive result even arithetic right shift
would be ok, and we could also in the match.pd pattern use convert? or
nop_convert? to allow signed multiplication with a cast to unsigned after
it, but guess it isn't that important and we can just go with TYPE_UNSIGNED
requirement for now.

> >> -  if (TREE_CODE (ctor) == STRING_CST)
> >> +  if (TREE_CODE (ctor) == STRING_CST && TYPE_PRECISION (type) == 8)
> >
> > Isn't another precondition that BITS_PER_UNIT is 8 (because STRING_CSTs are
> > really bytes)?
> 
> I've used CHAR_TYPE_SIZE instead of 8, but I don't think GCC supports anything other
> than BITS_PER_UNIT == 8 and CHAR_TYPE_SIZE == 8. GCC uses memcmp/strlen
> on STRING_CST (as well as direct accesses as 'char') which won't work if the host and
> target chars are not the same size.

Yes, we even don't support anything else ATM, but are trying to document in
the source when we rely on it.
So e.g. VIEW_CONVERT_EXPR folding has
  /* Check that the host and target are sane.  */
  if (CHAR_BIT != 8 || BITS_PER_UNIT != 8)
    return NULL_TREE;
etc.
> 2020-01-15  Wilco Dijkstra  <wdijkstr@arm.com>
> 
> 	PR tree-optimization/93231
> 	* tree-ssa-forwprop.c
> 	(optimize_count_trailing_zeroes): Check input_type is unsigned.

No need to wrap line before (optimize_count_trailing_zeroes), please use:
	* tree-ssa-forwprop.c (optimize_count_trailing_zeroes): Check
	input_type is unsigned.

> +int ctz2 (unsigned x)
> +{
> +  const int u = 0;
> +  static short table[64] =
> +    {
> +      32, 0, 1,12, 2, 6, u,13, 3, u, 7, u, u, u, u,14,
> +      10, 4, u, u, 8, u, u,25, u, u, u, u, u,21,27,15,
> +      31,11, 5, u, u, u, u, u, 9, u, u,24, u, u,20,26,
> +      30, u, u, u, u,23, u,19,29, u,22,18,28,17,16, u

This is just a GNU extension that we accept it in C, more portable would be
  enum U { u = 0; };
or #define u 0
But no big deal.

> @@ -1894,7 +1894,7 @@ optimize_count_trailing_zeroes (tree array_ref, tree x, tree mulc,
>    if (TREE_CODE (ctor) == CONSTRUCTOR)
>      return check_ctz_array (ctor, val, zero_val, shiftval, input_bits);
>  
> -  if (TREE_CODE (ctor) == STRING_CST)
> +  if (TREE_CODE (ctor) == STRING_CST && TYPE_PRECISION (type) == CHAR_TYPE_SIZE)

Too long line, please wrap.

Ok for trunk with those nits tweaked.

	Jakub