Jakub Jelinek <ja...@redhat.com> writes:
> On Tue, Feb 16, 2021 at 01:09:43PM +0000, Richard Sandiford wrote:
>> Can I put in a plea to put this in recog.[hc], and possibly also make
>> it a copy constructor for recog_data_d? I can't think of any legitimate
>> cases in which we'd want to copy the whole structure, instead of just
>> the active parts.
>
> Good idea. So like this?
>
> 2021-02-16 Jakub Jelinek <ja...@redhat.com>
>
> PR target/99104
> * recog.h (recog_data_d::recog_data_d ()): New defaulted default ctor.
> (recog_data_d::recog_data_d (const recog_data_d &)): Declare.
> (recog_data_d::operator = (const recog_data_d &)): Declare.
> * recog.c (recog_data_d::operator = (const recog_data_d &)): New
> assignment operator.
> (recog_data_d::recog_data_d (const recog_data_d &)): New copy ctor.
OK, thanks.
Richard
> * config/i386/i386.c (distance_non_agu_define): Don't call
> extract_insn_cached here.
> (ix86_lea_outperforms): Save and restore recog_data around call
> to distance_non_agu_define and distance_agu_use.
> (ix86_ok_to_clobber_flags): Remove.
> (ix86_avoid_lea_for_add): Don't call ix86_ok_to_clobber_flags.
> (ix86_avoid_lea_for_addr): Likewise. Adjust function comment.
> * config/i386/i386.m (*lea<mode>): Change from define_insn_and_split
> into define_insn. Move the splitting to define_peephole2 and
> check there using peep2_regno_dead_p if FLAGS_REG is dead.
>
> * gcc.dg/pr99104.c: New test.
>
> --- gcc/recog.h.jj 2021-01-15 19:04:42.436445517 +0100
> +++ gcc/recog.h 2021-02-16 14:26:00.384934711 +0100
> @@ -372,6 +372,12 @@ struct recog_data_d
>
> /* In case we are caching, hold insn data was generated for. */
> rtx_insn *insn;
> +
> + recog_data_d () = default;
> + /* Copy constructor and assignment operator, so that when copying the
> + structure we copy only the active elements. */
> + recog_data_d (const recog_data_d &);
> + recog_data_d &operator = (const recog_data_d &);
> };
>
> extern struct recog_data_d recog_data;
> --- gcc/recog.c.jj 2021-02-16 08:57:21.277960594 +0100
> +++ gcc/recog.c 2021-02-16 14:26:42.235464536 +0100
> @@ -109,7 +109,41 @@ init_recog (void)
> {
> volatile_ok = 1;
> }
> +
> +/* recog_data_d assignment operator, so that recog_data_d copying
> + only copies the active elements. */
> +
> +recog_data_d &
> +recog_data_d::operator = (const recog_data_d &src)
> +{
> + n_operands = src.n_operands;
> + n_dups = src.n_dups;
> + n_alternatives = src.n_alternatives;
> + is_asm = src.is_asm;
> + insn = src.insn;
> + for (int i = 0; i < src.n_operands; i++)
> + {
> + operand[i] = src.operand[i];
> + operand_loc[i] = src.operand_loc[i];
> + constraints[i] = src.constraints[i];
> + is_operator[i] = src.is_operator[i];
> + operand_mode[i] = src.operand_mode[i];
> + operand_type[i] = src.operand_type[i];
> + }
> + for (int i = 0; i < src.n_dups; i++)
> + {
> + dup_loc[i] = src.dup_loc[i];
> + dup_num[i] = src.dup_num[i];
> + }
> + return *this;
> +}
>
> +/* recog_data_d copy constructor, so that recog_data_d construction
> + only copies the active elements. */
> +recog_data_d::recog_data_d (const recog_data_d &src)
> +{
> + *this = src;
> +}
>
> /* Return true if labels in asm operands BODY are LABEL_REFs. */
>
> --- gcc/config/i386/i386.c.jj 2021-02-16 08:58:55.197890195 +0100
> +++ gcc/config/i386/i386.c 2021-02-16 14:20:19.061764891 +0100
> @@ -14754,11 +14754,6 @@ distance_non_agu_define (unsigned int re
> }
> }
>
> - /* get_attr_type may modify recog data. We want to make sure
> - that recog data is valid for instruction INSN, on which
> - distance_non_agu_define is called. INSN is unchanged here. */
> - extract_insn_cached (insn);
> -
> if (!found)
> return -1;
>
> @@ -14928,17 +14923,15 @@ ix86_lea_outperforms (rtx_insn *insn, un
> return true;
> }
>
> - rtx_insn *rinsn = recog_data.insn;
> + /* Remember recog_data content. */
> + struct recog_data_d recog_data_save = recog_data;
>
> dist_define = distance_non_agu_define (regno1, regno2, insn);
> dist_use = distance_agu_use (regno0, insn);
>
> - /* distance_non_agu_define can call extract_insn_cached. If this function
> - is called from define_split conditions, that can break insn splitting,
> - because split_insns works by clearing recog_data.insn and then modifying
> - recog_data.operand array and match the various split conditions. */
> - if (recog_data.insn != rinsn)
> - recog_data.insn = NULL;
> + /* distance_non_agu_define can call get_attr_type which can call
> + recog_memoized, restore recog_data back to previous content. */
> + recog_data = recog_data_save;
>
> if (dist_define < 0 || dist_define >= LEA_MAX_STALL)
> {
> @@ -14968,38 +14961,6 @@ ix86_lea_outperforms (rtx_insn *insn, un
> return dist_define >= dist_use;
> }
>
> -/* Return true if it is legal to clobber flags by INSN and
> - false otherwise. */
> -
> -static bool
> -ix86_ok_to_clobber_flags (rtx_insn *insn)
> -{
> - basic_block bb = BLOCK_FOR_INSN (insn);
> - df_ref use;
> - bitmap live;
> -
> - while (insn)
> - {
> - if (NONDEBUG_INSN_P (insn))
> - {
> - FOR_EACH_INSN_USE (use, insn)
> - if (DF_REF_REG_USE_P (use) && DF_REF_REGNO (use) == FLAGS_REG)
> - return false;
> -
> - if (insn_defines_reg (FLAGS_REG, INVALID_REGNUM, insn))
> - return true;
> - }
> -
> - if (insn == BB_END (bb))
> - break;
> -
> - insn = NEXT_INSN (insn);
> - }
> -
> - live = df_get_live_out(bb);
> - return !REGNO_REG_SET_P (live, FLAGS_REG);
> -}
> -
> /* Return true if we need to split op0 = op1 + op2 into a sequence of
> move and add to avoid AGU stalls. */
>
> @@ -15012,10 +14973,6 @@ ix86_avoid_lea_for_add (rtx_insn *insn,
> if (!TARGET_OPT_AGU || optimize_function_for_size_p (cfun))
> return false;
>
> - /* Check it is correct to split here. */
> - if (!ix86_ok_to_clobber_flags(insn))
> - return false;
> -
> regno0 = true_regnum (operands[0]);
> regno1 = true_regnum (operands[1]);
> regno2 = true_regnum (operands[2]);
> @@ -15051,7 +15008,7 @@ ix86_use_lea_for_mov (rtx_insn *insn, rt
> }
>
> /* Return true if we need to split lea into a sequence of
> - instructions to avoid AGU stalls. */
> + instructions to avoid AGU stalls during peephole2. */
>
> bool
> ix86_avoid_lea_for_addr (rtx_insn *insn, rtx operands[])
> @@ -15071,10 +15028,6 @@ ix86_avoid_lea_for_addr (rtx_insn *insn,
> && REG_P (XEXP (operands[1], 0))))
> return false;
>
> - /* Check if it is OK to split here. */
> - if (!ix86_ok_to_clobber_flags (insn))
> - return false;
> -
> ok = ix86_decompose_address (operands[1], &parts);
> gcc_assert (ok);
>
> --- gcc/config/i386/i386.md.jj 2021-01-13 10:12:07.036506101 +0100
> +++ gcc/config/i386/i386.md 2021-02-16 13:48:13.650317709 +0100
> @@ -5176,7 +5176,7 @@ (define_expand "floatunsdidf2"
>
> ;; Load effective address instructions
>
> -(define_insn_and_split "*lea<mode>"
> +(define_insn "*lea<mode>"
> [(set (match_operand:SWI48 0 "register_operand" "=r")
> (match_operand:SWI48 1 "address_no_seg_operand" "Ts"))]
> "ix86_hardreg_mov_ok (operands[0], operands[1])"
> @@ -5189,38 +5189,36 @@ (define_insn_and_split "*lea<mode>"
> else
> return "lea{<imodesuffix>}\t{%E1, %0|%0, %E1}";
> }
> - "reload_completed && ix86_avoid_lea_for_addr (insn, operands)"
> + [(set_attr "type" "lea")
> + (set (attr "mode")
> + (if_then_else
> + (match_operand 1 "SImode_address_operand")
> + (const_string "SI")
> + (const_string "<MODE>")))])
> +
> +(define_peephole2
> + [(set (match_operand:SWI48 0 "register_operand")
> + (match_operand:SWI48 1 "address_no_seg_operand"))]
> + "ix86_hardreg_mov_ok (operands[0], operands[1])
> + && peep2_regno_dead_p (0, FLAGS_REG)
> + && ix86_avoid_lea_for_addr (peep2_next_insn (0), operands)"
> [(const_int 0)]
> {
> machine_mode mode = <MODE>mode;
> - rtx pat;
> -
> - /* ix86_avoid_lea_for_addr re-recognizes insn and may
> - change operands[] array behind our back. */
> - pat = PATTERN (curr_insn);
> -
> - operands[0] = SET_DEST (pat);
> - operands[1] = SET_SRC (pat);
>
> /* Emit all operations in SImode for zero-extended addresses. */
> if (SImode_address_operand (operands[1], VOIDmode))
> mode = SImode;
>
> - ix86_split_lea_for_addr (curr_insn, operands, mode);
> + ix86_split_lea_for_addr (peep2_next_insn (0), operands, mode);
>
> /* Zero-extend return register to DImode for zero-extended addresses. */
> if (mode != <MODE>mode)
> - emit_insn (gen_zero_extendsidi2
> - (operands[0], gen_lowpart (mode, operands[0])));
> + emit_insn (gen_zero_extendsidi2 (operands[0],
> + gen_lowpart (mode, operands[0])));
>
> DONE;
> -}
> - [(set_attr "type" "lea")
> - (set (attr "mode")
> - (if_then_else
> - (match_operand 1 "SImode_address_operand")
> - (const_string "SI")
> - (const_string "<MODE>")))])
> +})
>
> ;; Add instructions
>
> --- gcc/testsuite/gcc.dg/pr99104.c.jj 2021-02-16 11:47:08.793255200 +0100
> +++ gcc/testsuite/gcc.dg/pr99104.c 2021-02-16 11:47:08.793255200 +0100
> @@ -0,0 +1,15 @@
> +/* PR target/99104 */
> +/* { dg-do compile { target int128 } } */
> +/* { dg-options "-O2 -fsel-sched-pipelining -fselective-scheduling2
> -funroll-loops" } */
> +
> +__int128 a;
> +int b;
> +int foo (void);
> +
> +int __attribute__ ((simd))
> +bar (void)
> +{
> + a = ~a;
> + if (foo ())
> + b = 0;
> +}
>
>
> Jakub
