Re: [PATCH V3 1/4] rs6000: build constant via li;rotldi

[Segher Boessenkool <segher@kernel.crashing.org>]

Hi!

On Fri, Jun 16, 2023 at 04:34:12PM +0800, Jiufu Guo wrote:
> +/* Check if value C can be built by 2 instructions: one is 'li', another is
> +   rotldi.
> +
> +   If so, *SHIFT is set to the shift operand of rotldi(rldicl), and *MASK
> +   is set to -1, and return true.  Return false otherwise.  */

Don't say "is set to -1", the point of having this is so you say "is set
to the "li" value".  Just like you describe what SHIFT is for.

> +static bool
> +can_be_built_by_li_and_rotldi (HOST_WIDE_INT c, int *shift,
> +				   HOST_WIDE_INT *mask)
> +{
> +  int n;

Put shis later, like:

> +  /* Check if C can be rotated to a positive or negative value
> +      which 'li' instruction is able to load.  */
  int n;
> +  if (can_be_rotated_to_lowbits (c, 15, &n)
> +      || can_be_rotated_to_lowbits (~c, 15, &n))
> +    {
> +      *mask = HOST_WIDE_INT_M1;
> +      *shift = HOST_BITS_PER_WIDE_INT - n;
> +      return true;
> +    }

It is tricky to see ~c will always work, since what is really done is -c
instead.  Can you just use that here?

> @@ -10266,15 +10291,14 @@ static void
>  rs6000_emit_set_long_const (rtx dest, HOST_WIDE_INT c)
>  {
>    rtx temp;
> +  int shift;
> +  HOST_WIDE_INT mask;
>    HOST_WIDE_INT ud1, ud2, ud3, ud4;
>  
>    ud1 = c & 0xffff;
> -  c = c >> 16;
> -  ud2 = c & 0xffff;
> -  c = c >> 16;
> -  ud3 = c & 0xffff;
> -  c = c >> 16;
> -  ud4 = c & 0xffff;
> +  ud2 = (c >> 16) & 0xffff;
> +  ud3 = (c >> 32) & 0xffff;
> +  ud4 = (c >> 48) & 0xffff;
>  
>    if ((ud4 == 0xffff && ud3 == 0xffff && ud2 == 0xffff && (ud1 & 0x8000))
>        || (ud4 == 0 && ud3 == 0 && ud2 == 0 && ! (ud1 & 0x8000)))
> @@ -10305,6 +10329,17 @@ rs6000_emit_set_long_const (rtx dest, HOST_WIDE_INT c)
>        emit_move_insn (dest, gen_rtx_XOR (DImode, temp,
>  					 GEN_INT ((ud2 ^ 0xffff) << 16)));
>      }
> +  else if (can_be_built_by_li_and_rotldi (c, &shift, &mask))
> +    {
> +      temp = !can_create_pseudo_p () ? dest : gen_reg_rtx (DImode);
> +      unsigned HOST_WIDE_INT imm = (c | ~mask);
> +      imm = (imm >> shift) | (imm << (HOST_BITS_PER_WIDE_INT - shift));
> +
> +      emit_move_insn (temp, GEN_INT (imm));
> +      if (shift != 0)
> +	temp = gen_rtx_ROTATE (DImode, temp, GEN_INT (shift));
> +      emit_move_insn (dest, temp);
> +    }

If you would rewrite so it isn't such a run-on thing with "else if",
instead using early outs, or even some factoring, you could declare the
variable used only in a tiny scope in that tiny scope instead.

> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/const-build.c
> @@ -0,0 +1,54 @@
> +/* { dg-do run } */
> +/* { dg-options "-O2 -save-temps" } */
> +/* { dg-require-effective-target has_arch_ppc64 } */

Please put a tiny comment here saying what this test is *for*?  The file
name is a bit of hint already, but you can indicate much more in one or
two lines :-)

With those adjustments, okay for trunk.  Thanks!

(If -c doesn't work, it needs more explanation).


Segher