Message ID | Y5hD4qnv/ddcKxyQ@tucnak |
---|---|
State | New |
Headers | show |
Series | i386: Fix up *concat*_{5,6,7} patterns [PR108044] | expand |
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
--- 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); +}