Re: [ARM Patch 2/n]PR53447: optimizations of 64bit ALU operation with constant

[Ramana Radhakrishnan <ramana dot radhakrishnan at linaro dot org>]

On 28 May 2012 11:08, Carrot Wei <carrot@google.com> wrote:
> Hi
>
> This is the second part of the patches that deals with 64bit and. It directly
> extends the patterns anddi3, anddi3_insn and anddi3_neon to handle 64bit
> constant operands.
>

Comments about const_di_ok_for_op still apply from my review of your add patch.

However I don't see why and /ior / xor with constants that have either
the low or high parts set can't be expanded directly into ands of
subregs with moves of zero's or the original value especially if you
aren't looking at doing 64 bit operations in neon .With Neon being
used for 64 bit arithmetic it gets more interesting.

Finally this should target PR target/53189.


Ramana

> Tested on arm qemu without regression.
>
> OK for trunk?
>
> thanks
> Carrot
>
> 2012-05-28 ?Wei Guozhi ?<carrot@google.com>
>
> ? ? ? ?PR target/53447
> ? ? ? ?* gcc.target/arm/pr53447-2.c: New testcase.
>
>
> 2012-05-28 ?Wei Guozhi ?<carrot@google.com>
>
> ? ? ? ?PR target/53447
> ? ? ? ?* config/arm/arm-protos.h (const_ok_for_anddi): New prototype.
> ? ? ? ?* config/arm/arm.c (const_ok_for_anddi): New function.
> ? ? ? ?* config/arm/constraints.md (De): New constraint.
> ? ? ? ?* config/arm/predicates.md (arm_anddi_operand): New predicate.
> ? ? ? ?(arm_immediate_anddi_operand): Likewise.
> ? ? ? ?(anddi_operand): Likewise.
> ? ? ? ?* config/arm/arm.md (anddi3): Extend it to handle 64bit constants.
> ? ? ? ?(anddi3_insn): Likewise.
> ? ? ? ?* config/arm/neon.md (anddi3_neon): Likewise.
>
>
>
> Index: testsuite/gcc.target/arm/pr53447-2.c
> ===================================================================
> --- testsuite/gcc.target/arm/pr53447-2.c ? ? ? ?(revision 0)
> +++ testsuite/gcc.target/arm/pr53447-2.c ? ? ? ?(revision 0)
> @@ -0,0 +1,8 @@
> +/* { dg-options "-O2" } ?*/
> +/* { dg-require-effective-target arm32 } */
> +/* { dg-final { scan-assembler-not "mov" } } */
> +
> +void t0p(long long * p)
> +{
> + ?*p &= 0x100000002;
> +}
> Index: config/arm/arm.c
> ===================================================================
> --- config/arm/arm.c ? ?(revision 187927)
> +++ config/arm/arm.c ? ?(working copy)
> @@ -2497,6 +2497,18 @@
> ? ? }
> ?}
>
> +/* Return TRUE if int I is a valid immediate constant used by pattern
> + ? anddi3_insn. ?*/
> +int
> +const_ok_for_anddi (HOST_WIDE_INT i)
> +{
> + ?HOST_WIDE_INT high = ARM_SIGN_EXTEND ((i >> 32) & 0xFFFFFFFF);
> + ?HOST_WIDE_INT low = ARM_SIGN_EXTEND (i & 0xFFFFFFFF);
> +
> + ?return (TARGET_32BIT && (const_ok_for_arm (low) || const_ok_for_arm (~low))
> + ? ? ? ? && (const_ok_for_arm (high) || const_ok_for_arm (~high)));
> +}
> +
> ?/* Emit a sequence of insns to handle a large constant.
> ? ?CODE is the code of the operation required, it can be any of SET, PLUS,
> ? ?IOR, AND, XOR, MINUS;
> Index: config/arm/arm-protos.h
> ===================================================================
> --- config/arm/arm-protos.h ? ? (revision 187927)
> +++ config/arm/arm-protos.h ? ? (working copy)
> @@ -47,6 +47,7 @@
> ?extern bool arm_small_register_classes_for_mode_p (enum machine_mode);
> ?extern int arm_hard_regno_mode_ok (unsigned int, enum machine_mode);
> ?extern bool arm_modes_tieable_p (enum machine_mode, enum machine_mode);
> +extern int const_ok_for_anddi (HOST_WIDE_INT);
> ?extern int const_ok_for_arm (HOST_WIDE_INT);
> ?extern int const_ok_for_op (HOST_WIDE_INT, enum rtx_code);
> ?extern int arm_split_constant (RTX_CODE, enum machine_mode, rtx,
> Index: config/arm/neon.md
> ===================================================================
> --- config/arm/neon.md ?(revision 187927)
> +++ config/arm/neon.md ?(working copy)
> @@ -774,9 +774,9 @@
> ?)
>
> ?(define_insn "anddi3_neon"
> - ?[(set (match_operand:DI 0 "s_register_operand" "=w,w,?&r,?&r,?w,?w")
> - ? ? ? ?(and:DI (match_operand:DI 1 "s_register_operand" "%w,0,0,r,w,0")
> - ? ? ? ? ? ? ? (match_operand:DI 2 "neon_inv_logic_op2" "w,DL,r,r,w,DL")))]
> + ?[(set (match_operand:DI 0 "s_register_operand" "=w,w,?&r,?&r,?w,?w,?&r,?&r")
> + ? ? ? ?(and:DI (match_operand:DI 1 "s_register_operand" "%w,0,0,r,w,0,0,r")
> + ? ? ? ? ? ? ? (match_operand:DI 2 "anddi_operand" "w,DL,r,r,w,DL,De,De")))]
> ? "TARGET_NEON"
> ?{
> ? switch (which_alternative)
> @@ -788,12 +788,14 @@
> ? ? ? ? ? ? ? ? ? ? DImode, 1, VALID_NEON_QREG_MODE (DImode));
> ? ? case 2: return "#";
> ? ? case 3: return "#";
> + ? ?case 6: return "#";
> + ? ?case 7: return "#";
> ? ? default: gcc_unreachable ();
> ? ? }
> ?}
> - ?[(set_attr "neon_type" "neon_int_1,neon_int_1,*,*,neon_int_1,neon_int_1")
> - ? (set_attr "length" "*,*,8,8,*,*")
> - ? (set_attr "arch" "nota8,nota8,*,*,onlya8,onlya8")]
> + ?[(set_attr "neon_type" "neon_int_1,neon_int_1,*,*,neon_int_1,neon_int_1,*,*")
> + ? (set_attr "length" "*,*,8,8,*,*,8,8")
> + ? (set_attr "arch" "nota8,nota8,*,*,onlya8,onlya8,*,*")]
> ?)
>
> ?(define_insn "orn<mode>3_neon"
> Index: config/arm/constraints.md
> ===================================================================
> --- config/arm/constraints.md ? (revision 187927)
> +++ config/arm/constraints.md ? (working copy)
> @@ -29,7 +29,7 @@
> ?;; in Thumb-1 state: I, J, K, L, M, N, O
>
> ?;; The following multi-letter normal constraints have been used:
> -;; in ARM/Thumb-2 state: Da, Db, Dc, Dn, Dl, DL, Dv, Dy, Di, Dt, Dz
> +;; in ARM/Thumb-2 state: Da, Db, Dc, De, Dn, Dl, DL, Dv, Dy, Di, Dt, Dz
> ?;; in Thumb-1 state: Pa, Pb, Pc, Pd, Pe
> ?;; in Thumb-2 state: Pj, PJ, Ps, Pt, Pu, Pv, Pw, Px, Py
>
> @@ -251,6 +251,12 @@
> ? ? ? (match_test "TARGET_32BIT && arm_const_double_inline_cost (op) == 4
> ? ? ? ? ? ? ? ? ? && !(optimize_size || arm_ld_sched)")))
>
> +(define_constraint "De"
> + "@internal
> + ?In ARM/Thumb-2 state a const_int that can be used by anddi3_insn. ?"
> + (and (match_code "const_int")
> + ? ? ?(match_test "TARGET_32BIT && const_ok_for_anddi (ival)")))
> +
> ?(define_constraint "Di"
> ?"@internal
> ? In ARM/Thumb-2 state a const_int or const_double where both the high
> Index: config/arm/predicates.md
> ===================================================================
> --- config/arm/predicates.md ? ?(revision 187927)
> +++ config/arm/predicates.md ? ?(working copy)
> @@ -117,6 +117,14 @@
> ? (and (match_code "const_int,const_double")
> ? ? ? ?(match_test "arm_const_double_by_immediates (op)")))
>
> +(define_predicate "arm_immediate_anddi_operand"
> + ?(and (match_code "const_int")
> + ? ? ? (match_test "const_ok_for_anddi (INTVAL (op))")))
> +
> +(define_predicate "arm_anddi_operand"
> + ?(ior (match_operand 0 "arm_immediate_anddi_operand")
> + ? ? ? (match_operand 0 "s_register_operand")))
> +
> ?(define_predicate "arm_neg_immediate_operand"
> ? (and (match_code "const_int")
> ? ? ? ?(match_test "const_ok_for_arm (-INTVAL (op))")))
> @@ -551,6 +559,10 @@
> ? (ior (match_operand 0 "imm_for_neon_inv_logic_operand")
> ? ? ? ?(match_operand 0 "s_register_operand")))
>
> +(define_predicate "anddi_operand"
> + ?(ior (match_operand 0 "neon_inv_logic_op2")
> + ? ? ? (match_operand 0 "arm_anddi_operand")))
> +
> ?;; TODO: We could check lane numbers more precisely based on the mode.
> ?(define_predicate "neon_lane_number"
> ? (and (match_code "const_int")
> Index: config/arm/arm.md
> ===================================================================
> --- config/arm/arm.md ? (revision 187927)
> +++ config/arm/arm.md ? (working copy)
> @@ -2072,17 +2072,38 @@
> ?(define_expand "anddi3"
> ? [(set (match_operand:DI ? ? ? ? 0 "s_register_operand" "")
> ? ? ? ?(and:DI (match_operand:DI 1 "s_register_operand" "")
> - ? ? ? ? ? ? ? (match_operand:DI 2 "neon_inv_logic_op2" "")))]
> + ? ? ? ? ? ? ? (match_operand:DI 2 "anddi_operand" "")))]
> ? "TARGET_32BIT"
> ? ""
> ?)
>
> -(define_insn "*anddi3_insn"
> - ?[(set (match_operand:DI ? ? ? ? 0 "s_register_operand" "=&r,&r")
> - ? ? ? (and:DI (match_operand:DI 1 "s_register_operand" ?"%0,r")
> - ? ? ? ? ? ? ? (match_operand:DI 2 "s_register_operand" ? "r,r")))]
> +(define_insn_and_split "*anddi3_insn"
> + ?[(set (match_operand:DI ? ? ? ? 0 "s_register_operand" "=&r,&r,&r,&r")
> + ? ? ? (and:DI (match_operand:DI 1 "s_register_operand" ?"%0,r,0,r")
> + ? ? ? ? ? ? ? (match_operand:DI 2 "arm_anddi_operand" ? "r,r,De,De")))]
> ? "TARGET_32BIT && !TARGET_IWMMXT && !TARGET_NEON"
> ? "#"
> + ?"TARGET_32BIT && !TARGET_IWMMXT && reload_completed"
> + ?[(set (match_dup 0) (and:SI (match_dup 1) (match_dup 2)))
> + ? (set (match_dup 3) (and:SI (match_dup 4) (match_dup 5)))]
> + ?"
> + ?{
> + ? ?operands[3] = gen_highpart (SImode, operands[0]);
> + ? ?operands[0] = gen_lowpart (SImode, operands[0]);
> + ? ?operands[4] = gen_highpart (SImode, operands[1]);
> + ? ?operands[1] = gen_lowpart (SImode, operands[1]);
> + ? ?if (GET_CODE (operands[2]) == CONST_INT)
> + ? ? ?{
> + ? ? ? HOST_WIDE_INT v = INTVAL (operands[2]);
> + ? ? ? operands[5] = GEN_INT (ARM_SIGN_EXTEND ((v >> 32) & 0xFFFFFFFF));
> + ? ? ? operands[2] = GEN_INT (ARM_SIGN_EXTEND (v & 0xFFFFFFFF));
> + ? ? ?}
> + ? ?else
> + ? ? ?{
> + ? ? ? operands[5] = gen_highpart (SImode, operands[2]);
> + ? ? ? operands[2] = gen_lowpart (SImode, operands[2]);
> + ? ? ?}
> + ?}"
> ? [(set_attr "length" "8")]
> ?)