On Tue, Feb 16, 2021 at 1:53 PM Jakub Jelinek <ja...@redhat.com> wrote:
>
> On Tue, Feb 16, 2021 at 12:39:39PM +0100, Uros Bizjak wrote:
> > > @@ -5197,7 +5209,7 @@ (define_insn_and_split "*lea<mode>"
> > >
> > > /* ix86_avoid_lea_for_addr re-recognizes insn and may
> > > change operands[] array behind our back. */
> > > - pat = PATTERN (curr_insn);
> > > + pat = PATTERN (peep2_next_insn (0));
> >
> > I remember there were several problems with the insn, so I introduced
> > the above hack. Probably, there is better solution for this...
>
> I'm afraid the define_peephole2 conditions are called with the same problem as
> define_split conditions, operands point into recog_data.operands.
>
> I wonder if instead of doing hacks like that we just couldn't save/restore
> recog_data variable around the
> dist_define = distance_non_agu_define (regno1, regno2, insn);
> dist_use = distance_agu_use (regno0, insn);
> calls.
>
> As in the following updated patch:
>
> 2021-02-16 Jakub Jelinek <ja...@redhat.com>
>
> PR target/99104
> * config/i386/i386.c (distance_non_agu_define): Don't call
> extract_insn_cached here.
> (copy_recog_data): New function.
> (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.
IMO copy_recog_data fits into generic code (and I'm sure generic code
use this new helper).
OK w/ or w/o the above change.
Thanks,
Uros.
> --- gcc/config/i386/i386.c.jj 2021-02-16 08:58:55.197890195 +0100
> +++ gcc/config/i386/i386.c 2021-02-16 13:47:15.500969539 +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;
>
> @@ -14897,6 +14892,32 @@ distance_agu_use (unsigned int regno0, r
> return distance >> 1;
> }
>
> +/* Copy recog_data_d from SRC to DEST. */
> +
> +static void
> +copy_recog_data (recog_data_d *dest, recog_data_d *src)
> +{
> + dest->n_operands = src->n_operands;
> + dest->n_dups = src->n_dups;
> + dest->n_alternatives = src->n_alternatives;
> + dest->is_asm = src->is_asm;
> + dest->insn = src->insn;
> + for (int i = 0; i < src->n_operands; i++)
> + {
> + dest->operand[i] = src->operand[i];
> + dest->operand_loc[i] = src->operand_loc[i];
> + dest->constraints[i] = src->constraints[i];
> + dest->is_operator[i] = src->is_operator[i];
> + dest->operand_mode[i] = src->operand_mode[i];
> + dest->operand_type[i] = src->operand_type[i];
> + }
> + for (int i = 0; i < src->n_dups; i++)
> + {
> + dest->dup_loc[i] = src->dup_loc[i];
> + dest->dup_num[i] = src->dup_num[i];
> + }
> +}
> +
> /* Define this macro to tune LEA priority vs ADD, it take effect when
> there is a dilemma of choosing LEA or ADD
> Negative value: ADD is more preferred than LEA
> @@ -14928,17 +14949,16 @@ 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_copy;
> + copy_recog_data (&recog_data_copy, &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. */
> + copy_recog_data (&recog_data, &recog_data_copy);
>
> if (dist_define < 0 || dist_define >= LEA_MAX_STALL)
> {
> @@ -14968,38 +14988,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 +15000,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 +15035,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 +15055,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
>
