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

  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.
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.


	Jakub
  

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
>
  

On Tue, Dec 13, 2022 at 01:21:54PM +0100, Uros Bizjak wrote:
> 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".

I was afraid of the PIC stuff in:
(define_constraint "n"
  "Matches a non-symbolic integer constant."
  (and (match_test "CONST_SCALAR_INT_P (op)")
       (match_test "!flag_pic || LEGITIMATE_PIC_OPERAND_P (op)")))
but now that I look at i386.cc (legitimate_pic_operand_p), you're right,
for CONST_INT and CONST_WIDE_INT it always returns true, so n is fine.

I'll retest with "n".

	Jakub