Re: [PATCH] Improve add/sub TImode double word splitters (PR rtl-optimization/70467)

[Uros Bizjak <ubizjak at gmail dot com>]

On Fri, Apr 1, 2016 at 3:23 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> The previous patch apparently isn't enough for TImode, because
> we don't even allow the CONST_WIDE_INT operands in there, it uses
> "e" constraint and similar predicate.  All we care about is that
> both of the words of the argument can be expressed as addq/adcq/subq/sbbq
> immediates, so this patch adds new predicates and new constraint for
> that purpose.  Suggestions for better names for those appreciated.

Maybe simply x86_64_hilo_int_operand, x86_64_hilo_general_operand and
<general_hilo_operand> to emphasize that the operand is specialized
for correct high/low part?

> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for stage1
> (while the previous patch looks simple enough that I'd like to see it in
> 6.x, this one IMHO can wait).

Yes, please. This is not a regression.

> 2016-04-01  Jakub Jelinek  <jakub@redhat.com>
>
>         PR rtl-optimization/70467
>         * config/i386/predicates.md (x86_64_double_int_operand,
>         x86_64_double_general_operand): New predicates.
>         * config/i386/constraints.md (Wd): New constraint.
>         * config/i386/i386.md (mode attr di): Use Wd instead of e.
>         (double_general_operand): New mode attr.
>         (add<mode>3, sub<mode>3): Use <double_general_operand>
>         instead of <general_operand>.
>         (*add<dwi>3_doubleword, *sub<dwi>3_doubleword): Use
>         x86_64_double_general_operand instead of <general_operand>.
>
>         * gcc.target/i386/pr70467-3.c: New test.
>         * gcc.target/i386/pr70467-4.c: New test.

OK.

Thanks,
Uros.

> --- gcc/config/i386/predicates.md.jj    2016-01-07 09:42:39.000000000 +0100
> +++ gcc/config/i386/predicates.md       2016-04-01 11:48:17.640386878 +0200
> @@ -332,6 +332,27 @@ (define_predicate "x86_64_zext_immediate
>    return false;
>  })
>
> +;; Return true if VALUE is a constant integer whose low and high words satisfy
> +;; x86_64_immediate_operand.
> +(define_predicate "x86_64_double_int_operand"
> +  (match_code "const_int,const_wide_int")
> +{
> +  switch (GET_CODE (op))
> +    {
> +    case CONST_INT:
> +      return x86_64_immediate_operand (op, mode);
> +    case CONST_WIDE_INT:

For now, we can get away with:

      gcc_assert (CONST_WIDE_INT_NUNITS (op) == 2);

      return (x86_64_immediate_operand (GEN_INT (CONST_WIDE_INT_ELT (op, 0)))
             && x86_64_immediate_operand (GEN_INT (CONST_WIDE_INT_ELT
(op, 1))));

This approach is used in several targets, not to mention rtlanal.c ;)

> +      return (x86_64_immediate_operand (simplify_subreg (DImode, op, TImode,
> +                                                        0), DImode)
> +             && x86_64_immediate_operand (simplify_subreg (DImode, op, TImode,
> +                                                           8), DImode));
> +
> +    default:
> +      gcc_unreachable ();
> +    }
> +})
> +
>  ;; Return true if size of VALUE can be stored in a sign
>  ;; extended immediate field.
>  (define_predicate "x86_64_immediate_size_operand"
> @@ -347,6 +368,14 @@ (define_predicate "x86_64_general_operan
>          (match_operand 0 "x86_64_immediate_operand"))
>      (match_operand 0 "general_operand")))
>
> +;; Return true if OP's both words are general operands representable
> +;; on x86_64.
> +(define_predicate "x86_64_double_general_operand"
> +  (if_then_else (match_test "TARGET_64BIT")
> +    (ior (match_operand 0 "nonimmediate_operand")
> +        (match_operand 0 "x86_64_double_int_operand"))
> +    (match_operand 0 "general_operand")))
> +
>  ;; Return true if OP is non-VOIDmode general operand representable
>  ;; on x86_64.  This predicate is used in sign-extending conversion
>  ;; operations that require non-VOIDmode immediate operands.
> --- gcc/config/i386/constraints.md.jj   2016-01-29 21:32:56.000000000 +0100
> +++ gcc/config/i386/constraints.md      2016-04-01 11:19:44.633921527 +0200
> @@ -266,6 +266,11 @@ (define_constraint "Wz"
>    (and (match_operand 0 "x86_64_zext_immediate_operand")
>         (match_test "GET_MODE (op) != VOIDmode")))
>
> +(define_constraint "Wd"
> +  "128-bit integer constant where both the high and low 64-bit word
> +   of it satisfies the e constraint."
> +  (match_operand 0 "x86_64_double_int_operand"))
> +
>  (define_constraint "Z"
>    "32-bit unsigned integer constant, or a symbolic reference known
>     to fit that range (for immediate operands in zero-extending x86-64
> --- gcc/config/i386/i386.md.jj  2016-03-31 17:33:36.000000000 +0200
> +++ gcc/config/i386/i386.md     2016-04-01 11:29:40.705729897 +0200
> @@ -1071,7 +1071,7 @@ (define_mode_attr i [(QI "n") (HI "n") (
>  (define_mode_attr g [(QI "qmn") (HI "rmn") (SI "rme") (DI "rme")])
>
>  ;; Immediate operand constraint for double integer modes.
> -(define_mode_attr di [(SI "nF") (DI "e")])
> +(define_mode_attr di [(SI "nF") (DI "Wd")])
>
>  ;; Immediate operand constraint for shifts.
>  (define_mode_attr S [(QI "I") (HI "I") (SI "I") (DI "J") (TI "O")])
> @@ -1084,6 +1084,15 @@ (define_mode_attr general_operand
>          (DI "x86_64_general_operand")
>          (TI "x86_64_general_operand")])
>
> +;; General operand predicate for integer modes, where for TImode
> +;; we need both words of the operand to be general operands.
> +(define_mode_attr double_general_operand
> +       [(QI "general_operand")
> +        (HI "general_operand")
> +        (SI "x86_64_general_operand")
> +        (DI "x86_64_general_operand")
> +        (TI "x86_64_double_general_operand")])
> +
>  ;; General sign extend operand predicate for integer modes,
>  ;; which disallows VOIDmode operands and thus it is suitable
>  ;; for use inside sign_extend.
> @@ -5423,7 +5432,7 @@ (define_insn_and_split "*lea<mode>"
>  (define_expand "add<mode>3"
>    [(set (match_operand:SDWIM 0 "nonimmediate_operand")
>         (plus:SDWIM (match_operand:SDWIM 1 "nonimmediate_operand")
> -                   (match_operand:SDWIM 2 "<general_operand>")))]
> +                   (match_operand:SDWIM 2 "<double_general_operand>")))]
>    ""
>    "ix86_expand_binary_operator (PLUS, <MODE>mode, operands); DONE;")
>
> @@ -5431,7 +5440,8 @@ (define_insn_and_split "*add<dwi>3_doubl
>    [(set (match_operand:<DWI> 0 "nonimmediate_operand" "=r,o")
>         (plus:<DWI>
>           (match_operand:<DWI> 1 "nonimmediate_operand" "%0,0")
> -         (match_operand:<DWI> 2 "<general_operand>" "ro<di>,r<di>")))
> +         (match_operand:<DWI> 2 "x86_64_double_general_operand"
> +                                                       "ro<di>,r<di>")))
>     (clobber (reg:CC FLAGS_REG))]
>    "ix86_binary_operator_ok (PLUS, <DWI>mode, operands)"
>    "#"
> @@ -6362,7 +6372,7 @@ (define_insn_and_split "*lea_general_4"
>  (define_expand "sub<mode>3"
>    [(set (match_operand:SDWIM 0 "nonimmediate_operand")
>         (minus:SDWIM (match_operand:SDWIM 1 "nonimmediate_operand")
> -                    (match_operand:SDWIM 2 "<general_operand>")))]
> +                    (match_operand:SDWIM 2 "<double_general_operand>")))]
>    ""
>    "ix86_expand_binary_operator (MINUS, <MODE>mode, operands); DONE;")
>
> @@ -6370,7 +6380,8 @@ (define_insn_and_split "*sub<dwi>3_doubl
>    [(set (match_operand:<DWI> 0 "nonimmediate_operand" "=r,o")
>         (minus:<DWI>
>           (match_operand:<DWI> 1 "nonimmediate_operand" "0,0")
> -         (match_operand:<DWI> 2 "<general_operand>" "ro<di>,r<di>")))
> +         (match_operand:<DWI> 2 "x86_64_double_general_operand"
> +                                                       "ro<di>,r<di>")))
>     (clobber (reg:CC FLAGS_REG))]
>    "ix86_binary_operator_ok (MINUS, <MODE>mode, operands)"
>    "#"
> --- gcc/testsuite/gcc.target/i386/pr70467-3.c.jj        2016-04-01 12:39:53.613101661 +0200
> +++ gcc/testsuite/gcc.target/i386/pr70467-3.c   2016-04-01 12:44:30.747330485 +0200
> @@ -0,0 +1,19 @@
> +/* PR rtl-optimization/70467 */
> +/* { dg-do compile { target int128 } } */
> +/* { dg-options "-O2" } */
> +
> +__uint128_t
> +foo (__uint128_t x)
> +{
> +  return x + ((__uint128_t) 123456 << 64);
> +}
> +
> +__uint128_t
> +bar (__uint128_t x)
> +{
> +  return x - ((__uint128_t) 123456 << 64);
> +}
> +
> +/* Make sure there are no unnecessary additions with carry.  */
> +/* { dg-final { scan-assembler-not "adcq\[^\n\r\]*%" } } */
> +/* { dg-final { scan-assembler-not "sbbq\[^\n\r\]*%" } } */
> --- gcc/testsuite/gcc.target/i386/pr70467-4.c.jj        2016-04-01 12:40:01.567993391 +0200
> +++ gcc/testsuite/gcc.target/i386/pr70467-4.c   2016-04-01 12:43:58.067775153 +0200
> @@ -0,0 +1,18 @@
> +/* PR rtl-optimization/70467 */
> +/* { dg-do compile { target int128 } } */
> +/* { dg-options "-O2" } */
> +
> +__uint128_t
> +foo (__uint128_t x)
> +{
> +  return x + ((__uint128_t) 123456 << 64) + 0x1234567;
> +}
> +
> +__uint128_t
> +bar (__uint128_t x)
> +{
> +  return x - ((__uint128_t) 123456 << 64) + 0x1234567;
> +}
> +
> +/* Make sure the immediates are not loaded into registers first.  */
> +/* { dg-final { scan-assembler-not "mov\[lq\]\[ \t\]*.\[0-9-\]" } } */
>
>         Jakub