Re: [PATCH] i386: Fix up *concat*_{5,6,7} patterns [PR108044]

[Uros Bizjak <ubizjak@gmail.com>]

On Tue, Dec 13, 2022 at 10:20 AM Jakub Jelinek <jakub@redhat.com> wrote:
>
> Hi!
>
> The following patch fixes 2 issues with the *concat<half><mode>3_5 and
> *concat<mode><dwi>3_{6,7} patterns.
> One is that if the destination is memory rather than register, then
> we can't use movabsq and so can't support all the possible immediates.
> I see 3 possibilities to fix that.  One would be to use
> x86_64_hilo_int_operand predicate instead of const_scalar_int_operand
> and thus not match it at all during combine in such cases, but that
> unnecessarily pessimizes also the case when it is loaded into register
> where we can use movabsq.
> Another one is what is implemented in the patch, use Wd constraint
> for the integer on 64-bit if destination is memory and X (didn't find
> more appropriate one which would accept any const_int/const_wide_int
> and the value checking is done in the conditions) otherwise.

Perhaps you should use "n" instead of "X".

> Yet another option would be to add match_scratch to the pattern and use
> it with =X constraints except for the =o case for 64-bit non-Wd where it
> would give a single DImode register (rather than 2).
>
> Another thing is that if one half of the constant is
> ix86_endbr_immediate_operand, then for -fcf-protection=branch we
> force those constants into memory and that might not work properly
> with -fpic.  So we should refuse to match with such constants.
> OT, seems for movabsq we don't check that and happily allow the endbr
> pattern in the immediate.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk,
> or do you prefer another way to do it (see above)?

> 2022-12-13  Jakub Jelinek  <jakub@redhat.com>
>
>         PR target/108044
>         * config/i386/i386.md (*concat<half><mode>3_5, *concat<mode><dwi>3_6,
>         *concat<mode><dwi>3_7): Split alternative with =ro output constraint
>         into =r,o,o and use Wd input constraint for the last alternative which
>         is enabled for TARGET_64BIT.  Reject ix86_endbr_immediate_operand
>         in the input constant.
>
>         * gcc.target/i386/pr108044-1.c: New test.
>         * gcc.target/i386/pr108044-2.c: New test.
>         * gcc.target/i386/pr108044-3.c: New test.
>         * gcc.target/i386/pr108044-4.c: New test.

OK with or without the change to "n" constraint, although I would
prefer "n", since "X" can perhaps result in some yet unknown
surprises.

Thanks,
Uros.
> --- gcc/config/i386/i386.md.jj  2022-12-08 14:55:38.807303856 +0100
> +++ gcc/config/i386/i386.md     2022-12-12 10:37:09.332995296 +0100
> @@ -11470,11 +11470,11 @@ (define_insn_and_split "*concat<mode><dw
>  })
>
>  (define_insn_and_split "*concat<half><mode>3_5"
> -  [(set (match_operand:DWI 0 "nonimmediate_operand" "=ro")
> +  [(set (match_operand:DWI 0 "nonimmediate_operand" "=r,o,o")
>         (any_or_plus:DWI
> -         (ashift:DWI (match_operand:DWI 1 "register_operand" "r")
> +         (ashift:DWI (match_operand:DWI 1 "register_operand" "r,r,r")
>                       (match_operand:DWI 2 "const_int_operand"))
> -         (match_operand:DWI 3 "const_scalar_int_operand")))]
> +         (match_operand:DWI 3 "const_scalar_int_operand" "X,X,Wd")))]
>    "INTVAL (operands[2]) == <MODE_SIZE> * BITS_PER_UNIT / 2
>     && (<MODE>mode == DImode
>         ? CONST_INT_P (operands[3])
> @@ -11482,7 +11482,12 @@ (define_insn_and_split "*concat<half><mo
>         : CONST_INT_P (operands[3])
>         ? INTVAL (operands[3]) >= 0
>         : CONST_WIDE_INT_NUNITS (operands[3]) == 2
> -        && CONST_WIDE_INT_ELT (operands[3], 1) == 0)"
> +        && CONST_WIDE_INT_ELT (operands[3], 1) == 0)
> +   && !(CONST_INT_P (operands[3])
> +       ? ix86_endbr_immediate_operand (operands[3], VOIDmode)
> +       : ix86_endbr_immediate_operand (GEN_INT (CONST_WIDE_INT_ELT (operands[3],
> +                                                                    0)),
> +                                       VOIDmode))"
>    "#"
>    "&& reload_completed"
>    [(clobber (const_int 0))]
> @@ -11491,16 +11496,17 @@ (define_insn_and_split "*concat<half><mo
>    split_double_concat (<MODE>mode, operands[0], op3,
>                        gen_lowpart (<HALF>mode, operands[1]));
>    DONE;
> -})
> +}
> +  [(set_attr "isa" "*,nox64,x64")])
>
>  (define_insn_and_split "*concat<mode><dwi>3_6"
> -  [(set (match_operand:<DWI> 0 "nonimmediate_operand" "=ro,r")
> +  [(set (match_operand:<DWI> 0 "nonimmediate_operand" "=r,o,o,r")
>         (any_or_plus:<DWI>
>           (ashift:<DWI>
>             (zero_extend:<DWI>
> -             (match_operand:DWIH 1 "nonimmediate_operand" "r,m"))
> +             (match_operand:DWIH 1 "nonimmediate_operand" "r,r,r,m"))
>             (match_operand:<DWI> 2 "const_int_operand"))
> -         (match_operand:<DWI> 3 "const_scalar_int_operand")))]
> +         (match_operand:<DWI> 3 "const_scalar_int_operand" "X,X,Wd,X")))]
>    "INTVAL (operands[2]) == <MODE_SIZE> * BITS_PER_UNIT
>     && (<DWI>mode == DImode
>         ? CONST_INT_P (operands[3])
> @@ -11508,7 +11514,12 @@ (define_insn_and_split "*concat<mode><dw
>         : CONST_INT_P (operands[3])
>         ? INTVAL (operands[3]) >= 0
>         : CONST_WIDE_INT_NUNITS (operands[3]) == 2
> -        && CONST_WIDE_INT_ELT (operands[3], 1) == 0)"
> +        && CONST_WIDE_INT_ELT (operands[3], 1) == 0)
> +   && !(CONST_INT_P (operands[3])
> +       ? ix86_endbr_immediate_operand (operands[3], VOIDmode)
> +       : ix86_endbr_immediate_operand (GEN_INT (CONST_WIDE_INT_ELT (operands[3],
> +                                                                    0)),
> +                                       VOIDmode))"
>    "#"
>    "&& reload_completed"
>    [(clobber (const_int 0))]
> @@ -11516,20 +11527,25 @@ (define_insn_and_split "*concat<mode><dw
>    rtx op3 = simplify_subreg (<MODE>mode, operands[3], <DWI>mode, 0);
>    split_double_concat (<DWI>mode, operands[0], op3, operands[1]);
>    DONE;
> -})
> +}
> +  [(set_attr "isa" "*,nox64,x64,*")])
>
>  (define_insn_and_split "*concat<mode><dwi>3_7"
> -  [(set (match_operand:<DWI> 0 "nonimmediate_operand" "=ro,r")
> +  [(set (match_operand:<DWI> 0 "nonimmediate_operand" "=r,o,o,r")
>         (any_or_plus:<DWI>
>           (zero_extend:<DWI>
> -           (match_operand:DWIH 1 "nonimmediate_operand" "r,m"))
> -         (match_operand:<DWI> 2 "const_scalar_int_operand")))]
> +           (match_operand:DWIH 1 "nonimmediate_operand" "r,r,r,m"))
> +         (match_operand:<DWI> 2 "const_scalar_int_operand" "X,X,Wd,X")))]
>    "<DWI>mode == DImode
>     ? CONST_INT_P (operands[2])
>       && (UINTVAL (operands[2]) & GET_MODE_MASK (SImode)) == 0
> +     && !ix86_endbr_immediate_operand (operands[2], VOIDmode)
>     : CONST_WIDE_INT_P (operands[2])
>       && CONST_WIDE_INT_NUNITS (operands[2]) == 2
> -     && CONST_WIDE_INT_ELT (operands[2], 0) == 0"
> +     && CONST_WIDE_INT_ELT (operands[2], 0) == 0
> +     && !ix86_endbr_immediate_operand (GEN_INT (CONST_WIDE_INT_ELT (operands[2],
> +                                                                   1)),
> +                                      VOIDmode)"
>    "#"
>    "&& reload_completed"
>    [(clobber (const_int 0))]
> @@ -11541,7 +11557,8 @@ (define_insn_and_split "*concat<mode><dw
>      op2 = gen_int_mode (CONST_WIDE_INT_ELT (operands[2], 1), <MODE>mode);
>    split_double_concat (<DWI>mode, operands[0], operands[1], op2);
>    DONE;
> -})
> +}
> +  [(set_attr "isa" "*,nox64,x64,*")])
>
>  ;; Negation instructions
>
> --- gcc/testsuite/gcc.target/i386/pr108044-1.c.jj       2022-12-12 10:25:23.664131494 +0100
> +++ gcc/testsuite/gcc.target/i386/pr108044-1.c  2022-12-12 10:20:02.395740622 +0100
> @@ -0,0 +1,33 @@
> +/* PR target/108044 */
> +/* { dg-do compile { target int128 } } */
> +/* { dg-options "-O2" } */
> +
> +static inline unsigned __int128
> +foo (unsigned long long x, unsigned long long y)
> +{
> +  return ((unsigned __int128) x << 64) | y;
> +}
> +
> +void
> +bar (unsigned __int128 *p, unsigned long long x)
> +{
> +  p[0] = foo (x, 0xdeadbeefcafebabeULL);
> +}
> +
> +void
> +baz (unsigned __int128 *p, unsigned long long x)
> +{
> +  p[0] = foo (0xdeadbeefcafebabeULL, x);
> +}
> +
> +void
> +qux (unsigned __int128 *p, unsigned long long x)
> +{
> +  p[0] = foo (x, 0xffffffffcafebabeULL);
> +}
> +
> +void
> +corge (unsigned __int128 *p, unsigned long long x)
> +{
> +  p[0] = foo (0xffffffffcafebabeULL, x);
> +}
> --- gcc/testsuite/gcc.target/i386/pr108044-2.c.jj       2022-12-12 10:25:27.069082645 +0100
> +++ gcc/testsuite/gcc.target/i386/pr108044-2.c  2022-12-12 10:19:06.545541879 +0100
> @@ -0,0 +1,21 @@
> +/* PR target/108044 */
> +/* { dg-do compile { target ia32 } } */
> +/* { dg-options "-O2" } */
> +
> +static inline unsigned long long
> +foo (unsigned int x, unsigned int y)
> +{
> +  return ((unsigned long long) x << 32) | y;
> +}
> +
> +void
> +bar (unsigned long long *p, unsigned int x)
> +{
> +  p[0] = foo (x, 0xcafebabeU);
> +}
> +
> +void
> +baz (unsigned long long *p, unsigned int x)
> +{
> +  p[0] = foo (0xcafebabeU, x);
> +}
> --- gcc/testsuite/gcc.target/i386/pr108044-3.c.jj       2022-12-12 10:27:08.348629628 +0100
> +++ gcc/testsuite/gcc.target/i386/pr108044-3.c  2022-12-12 10:29:19.967741328 +0100
> @@ -0,0 +1,33 @@
> +/* PR target/108044 */
> +/* { dg-do compile { target int128 } } */
> +/* { dg-options "-O2 -fcf-protection=branch" } */
> +
> +static inline unsigned __int128
> +foo (unsigned long long x, unsigned long long y)
> +{
> +  return ((unsigned __int128) x << 64) | y;
> +}
> +
> +unsigned __int128
> +bar (unsigned long long x)
> +{
> +  return foo (x, 0xfa1e0ff3ULL);
> +}
> +
> +unsigned __int128
> +baz (unsigned long long x)
> +{
> +  return foo (0xfa1e0ff3ULL, x);
> +}
> +
> +unsigned __int128
> +qux (unsigned long long x)
> +{
> +  return foo (x, 0xffbafa1e0ff3abdeULL);
> +}
> +
> +unsigned __int128
> +corge (unsigned long long x)
> +{
> +  return foo (0xffbafa1e0ff3abdeULL, x);
> +}
> --- gcc/testsuite/gcc.target/i386/pr108044-4.c.jj       2022-12-12 10:37:54.419345499 +0100
> +++ gcc/testsuite/gcc.target/i386/pr108044-4.c  2022-12-12 10:38:43.668635715 +0100
> @@ -0,0 +1,21 @@
> +/* PR target/108044 */
> +/* { dg-do compile { target ia32 } } */
> +/* { dg-options "-O2 -fcf-protection=branch" } */
> +
> +static inline unsigned long long
> +foo (unsigned int x, unsigned int y)
> +{
> +  return ((unsigned long long) x << 32) | y;
> +}
> +
> +unsigned long long
> +bar (unsigned int x)
> +{
> +  return foo (x, 0xfa1e0ff3U);
> +}
> +
> +unsigned long long
> +baz (unsigned int x)
> +{
> +  return foo (0xfa1e0ff3U, x);
> +}
>
>         Jakub
>