Re: [PATCH] i386: Fix up *_doubleword_mask [PR105825]

[Uros Bizjak <ubizjak@gmail.com>]

On Fri, Jun 3, 2022 at 12:17 PM Jakub Jelinek <jakub@redhat.com> wrote:
>
> Hi!
>
> My PR105778 patch apparently broke the following testcase.
> If the mask has the top relevant bit clear (i.e. we know we are shifting
> by 0 to wordsize bits - 1) but doesn't have all the bits below it set,
> we emit andsi3 before the shift sequence.  When the pattern had :SI
> for that operand, that was just fine, but now that it can be also HImode
> or for -m64 DImode, we either can use a lowpart or paradoxical subreg to
> SImode as the following patch, or we could use a HImode or DImode AND.

I think it is better to leave the operation in its natural mode and
leave the peephole pass to do its magic, depending on the target.

Uros.

> Ok for trunk if it passes bootstrap/regtest on x86_64-linux and i686-linux?
>
> 2022-06-03  Jakub Jelinek  <jakub@redhat.com>
>
>         PR target/105825
>         * config/i386/i386.md (*ashl<dwi>3_doubleword_mask,
>         *<insn><dwi>3_doubleword_mask): If top bit of mask is clear, but lower
>         bits of mask aren't all set and operands[2] doesn't have SImode,
>         force it to get and use subreg to SImode for andsi3 operand.
>
>         * gcc.dg/pr105825.c: New test.
>
> --- gcc/config/i386/i386.md.jj  2022-06-02 10:40:00.034660893 +0200
> +++ gcc/config/i386/i386.md     2022-06-03 12:00:39.323292767 +0200
> @@ -11935,6 +11935,11 @@ (define_insn_and_split "*ashl<dwi>3_doub
>        != ((<MODE_SIZE> * BITS_PER_UNIT) - 1))
>      {
>        rtx tem = gen_reg_rtx (SImode);
> +      if (GET_MODE (operands[2]) != SImode)
> +       {
> +         operands[2] = force_reg (GET_MODE (operands[2]), operands[2]);
> +         operands[2] = gen_lowpart (SImode, operands[2]);
> +       }
>        emit_insn (gen_andsi3 (tem, operands[2], operands[3]));
>        operands[2] = tem;
>      }
> @@ -12900,6 +12905,11 @@ (define_insn_and_split "*<insn><dwi>3_do
>        != ((<MODE_SIZE> * BITS_PER_UNIT) - 1))
>      {
>        rtx tem = gen_reg_rtx (SImode);
> +      if (GET_MODE (operands[2]) != SImode)
> +       {
> +         operands[2] = force_reg (GET_MODE (operands[2]), operands[2]);
> +         operands[2] = gen_lowpart (SImode, operands[2]);
> +       }
>        emit_insn (gen_andsi3 (tem, operands[2], operands[3]));
>        operands[2] = tem;
>      }
> --- gcc/testsuite/gcc.dg/pr105825.c.jj  2022-06-03 12:01:58.008460659 +0200
> +++ gcc/testsuite/gcc.dg/pr105825.c     2022-06-03 12:01:41.259637783 +0200
> @@ -0,0 +1,13 @@
> +/* PR target/105825 */
> +/* { dg-do compile { target int128 } } */
> +/* { dg-options "-O2" } */
> +/* { dg-additional-options "-mavx" { target avx } } */
> +
> +__int128 j;
> +int i;
> +
> +void
> +foo (void)
> +{
> +  j <<= __builtin_parityll (i);
> +}
>
>         Jakub
>