This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH][STV] More compile-time improvements


On Tue, Aug 27, 2019 at 2:14 PM Richard Biener <rguenther@suse.de> wrote:
>
>
> With forcing STV for all chains I run into the same issue as I
> fixed for the analysis phase which is looping over all defs
> of a pseudo rather just those interesting.  The following fixes
> this by recording insn/pseudo pairs in mark_dual_mode_def;
> well, not actually pairs but tracking insns in addition to
> pseudos which allows us to cheaply do the set property plus
> visit the defs that are interesting (assuming the number of
> defs on an insn is a lot less endless than the number of defs
> of a pseudo).
>
> This way the original testcase with STV forced goes down from
> 200s to 24s compile-time with
>
>  machine dep reorg                  :   7.09 ( 20%)   0.01 (  2%)   8.18 (
> 21%)    1854 kB (  2%)
>
> where almost all of the compile-time is spent in DFs
> deferred rescan processing.  I'm not sure we even need
> that but I'm not too eager to dig more into DF than necessary
> right now (we call df_finish () at the end which removes
> the DU/UD_CHAIN and the MD problem (what do we need that for!?).
> We don't appropriately scan all insn we add so the prevailing
> problems like LIVE are not updated, but well...
>
> Bootstrapped and tested on x86_64-unknown-linux-gnu with STV
> forced and for Haswell this time.
>
> OK?
>
> Thanks,
> Richard.
>
> 2019-08-27  Richard Biener  <rguenther@suse.de>
>
>         * config/i386/i386-features.h
>         (general_scalar_chain::~general_scalar_chain): Add.
>         (general_scalar_chain::insns_conv): New bitmap.
>         (general_scalar_chain::n_sse_to_integer): New.
>         (general_scalar_chain::n_integer_to_sse): Likewise.
>         (general_scalar_chain::make_vector_copies): Adjust signature.
>         * config/i386/i386-features.c
>         (general_scalar_chain::general_scalar_chain): Outline,
>         initialize new members.
>         (general_scalar_chain::~general_scalar_chain): New.
>         (general_scalar_chain::mark_dual_mode_def): Record insns
>         we need to insert conversions at and count them.
>         (general_scalar_chain::compute_convert_gain): Account
>         for conversion instructions at chain boundary.
>         (general_scalar_chain::make_vector_copies): Generate a single
>         copy for a def by a specific insn.
>         (general_scalar_chain::convert_registers): First populate
>         defs_map, then make copies at out-of chain insns.

LGTM.

Thanks,
Uros.

> Index: gcc/config/i386/i386-features.c
> ===================================================================
> --- gcc/config/i386/i386-features.c     (revision 274945)
> +++ gcc/config/i386/i386-features.c     (working copy)
> @@ -320,6 +320,20 @@ scalar_chain::add_to_queue (unsigned ins
>    bitmap_set_bit (queue, insn_uid);
>  }
>
> +general_scalar_chain::general_scalar_chain (enum machine_mode smode_,
> +                                           enum machine_mode vmode_)
> +     : scalar_chain (smode_, vmode_)
> +{
> +  insns_conv = BITMAP_ALLOC (NULL);
> +  n_sse_to_integer = 0;
> +  n_integer_to_sse = 0;
> +}
> +
> +general_scalar_chain::~general_scalar_chain ()
> +{
> +  BITMAP_FREE (insns_conv);
> +}
> +
>  /* For DImode conversion, mark register defined by DEF as requiring
>     conversion.  */
>
> @@ -328,15 +342,27 @@ general_scalar_chain::mark_dual_mode_def
>  {
>    gcc_assert (DF_REF_REG_DEF_P (def));
>
> -  if (bitmap_bit_p (defs_conv, DF_REF_REGNO (def)))
> -    return;
> -
> +  /* Record the def/insn pair so we can later efficiently iterate over
> +     the defs to convert on insns not in the chain.  */
> +  bool reg_new = bitmap_set_bit (defs_conv, DF_REF_REGNO (def));
> +  if (!bitmap_bit_p (insns, DF_REF_INSN_UID (def)))
> +    {
> +      if (!bitmap_set_bit (insns_conv, DF_REF_INSN_UID (def))
> +         && !reg_new)
> +       return;
> +      n_integer_to_sse++;
> +    }
> +  else
> +    {
> +      if (!reg_new)
> +       return;
> +      n_sse_to_integer++;
> +    }
> +
>    if (dump_file)
>      fprintf (dump_file,
>              "  Mark r%d def in insn %d as requiring both modes in chain #%d\n",
>              DF_REF_REGNO (def), DF_REF_INSN_UID (def), chain_id);
> -
> -  bitmap_set_bit (defs_conv, DF_REF_REGNO (def));
>  }
>
>  /* For TImode conversion, it is unused.  */
> @@ -523,7 +549,7 @@ general_scalar_chain::compute_convert_ga
>                || GET_CODE (src) == ASHIFTRT
>                || GET_CODE (src) == LSHIFTRT)
>         {
> -         if (CONST_INT_P (XEXP (src, 0)))
> +         if (CONST_INT_P (XEXP (src, 0)))
>             igain -= vector_const_cost (XEXP (src, 0));
>           igain += m * ix86_cost->shift_const - ix86_cost->sse_op;
>           if (INTVAL (XEXP (src, 1)) >= 32)
> @@ -588,9 +614,12 @@ general_scalar_chain::compute_convert_ga
>    if (dump_file)
>      fprintf (dump_file, "  Instruction conversion gain: %d\n", gain);
>
> -  /* ???  What about integer to SSE?  */
> -  EXECUTE_IF_SET_IN_BITMAP (defs_conv, 0, insn_uid, bi)
> -    cost += DF_REG_DEF_COUNT (insn_uid) * ix86_cost->sse_to_integer;
> +  /* Cost the integer to sse and sse to integer moves.  */
> +  cost += n_sse_to_integer * ix86_cost->sse_to_integer;
> +  /* ???  integer_to_sse but we only have that in the RA cost table.
> +     Assume sse_to_integer/integer_to_sse are the same which they
> +     are at the moment.  */
> +  cost += n_integer_to_sse * ix86_cost->sse_to_integer;
>
>    if (dump_file)
>      fprintf (dump_file, "  Registers conversion cost: %d\n", cost);
> @@ -649,85 +678,64 @@ gen_gpr_to_xmm_move_src (enum machine_mo
>     and replace its uses in a chain.  */
>
>  void
> -general_scalar_chain::make_vector_copies (unsigned regno)
> +general_scalar_chain::make_vector_copies (rtx_insn *insn, rtx reg)
>  {
> -  rtx reg = regno_reg_rtx[regno];
> -  rtx vreg = gen_reg_rtx (smode);
> -  df_ref ref;
> +  rtx vreg = *defs_map.get (reg);
>
> -  defs_map.put (reg, vreg);
> -
> -  /* For each insn defining REGNO, see if it is defined by an insn
> -     not part of the chain but with uses in insns part of the chain
> -     and insert a copy in that case.  */
> -  for (ref = DF_REG_DEF_CHAIN (regno); ref; ref = DF_REF_NEXT_REG (ref))
> +  start_sequence ();
> +  if (!TARGET_INTER_UNIT_MOVES_TO_VEC)
>      {
> -      if (bitmap_bit_p (insns, DF_REF_INSN_UID (ref)))
> -       continue;
> -      df_link *use;
> -      for (use = DF_REF_CHAIN (ref); use; use = use->next)
> -       if (!DF_REF_REG_MEM_P (use->ref)
> -           && bitmap_bit_p (insns, DF_REF_INSN_UID (use->ref)))
> -         break;
> -      if (!use)
> -       continue;
> -
> -      start_sequence ();
> -      if (!TARGET_INTER_UNIT_MOVES_TO_VEC)
> +      rtx tmp = assign_386_stack_local (smode, SLOT_STV_TEMP);
> +      if (smode == DImode && !TARGET_64BIT)
>         {
> -         rtx tmp = assign_386_stack_local (smode, SLOT_STV_TEMP);
> -         if (smode == DImode && !TARGET_64BIT)
> -           {
> -             emit_move_insn (adjust_address (tmp, SImode, 0),
> -                             gen_rtx_SUBREG (SImode, reg, 0));
> -             emit_move_insn (adjust_address (tmp, SImode, 4),
> -                             gen_rtx_SUBREG (SImode, reg, 4));
> -           }
> -         else
> -           emit_move_insn (copy_rtx (tmp), reg);
> -         emit_insn (gen_rtx_SET (gen_rtx_SUBREG (vmode, vreg, 0),
> -                                 gen_gpr_to_xmm_move_src (vmode, tmp)));
> +         emit_move_insn (adjust_address (tmp, SImode, 0),
> +                         gen_rtx_SUBREG (SImode, reg, 0));
> +         emit_move_insn (adjust_address (tmp, SImode, 4),
> +                         gen_rtx_SUBREG (SImode, reg, 4));
>         }
> -      else if (!TARGET_64BIT && smode == DImode)
> +      else
> +       emit_move_insn (copy_rtx (tmp), reg);
> +      emit_insn (gen_rtx_SET (gen_rtx_SUBREG (vmode, vreg, 0),
> +                             gen_gpr_to_xmm_move_src (vmode, tmp)));
> +    }
> +  else if (!TARGET_64BIT && smode == DImode)
> +    {
> +      if (TARGET_SSE4_1)
>         {
> -         if (TARGET_SSE4_1)
> -           {
> -             emit_insn (gen_sse2_loadld (gen_rtx_SUBREG (V4SImode, vreg, 0),
> -                                         CONST0_RTX (V4SImode),
> -                                         gen_rtx_SUBREG (SImode, reg, 0)));
> -             emit_insn (gen_sse4_1_pinsrd (gen_rtx_SUBREG (V4SImode, vreg, 0),
> -                                           gen_rtx_SUBREG (V4SImode, vreg, 0),
> -                                           gen_rtx_SUBREG (SImode, reg, 4),
> -                                           GEN_INT (2)));
> -           }
> -         else
> -           {
> -             rtx tmp = gen_reg_rtx (DImode);
> -             emit_insn (gen_sse2_loadld (gen_rtx_SUBREG (V4SImode, vreg, 0),
> -                                         CONST0_RTX (V4SImode),
> -                                         gen_rtx_SUBREG (SImode, reg, 0)));
> -             emit_insn (gen_sse2_loadld (gen_rtx_SUBREG (V4SImode, tmp, 0),
> -                                         CONST0_RTX (V4SImode),
> -                                         gen_rtx_SUBREG (SImode, reg, 4)));
> -             emit_insn (gen_vec_interleave_lowv4si
> -                        (gen_rtx_SUBREG (V4SImode, vreg, 0),
> -                         gen_rtx_SUBREG (V4SImode, vreg, 0),
> -                         gen_rtx_SUBREG (V4SImode, tmp, 0)));
> -           }
> +         emit_insn (gen_sse2_loadld (gen_rtx_SUBREG (V4SImode, vreg, 0),
> +                                     CONST0_RTX (V4SImode),
> +                                     gen_rtx_SUBREG (SImode, reg, 0)));
> +         emit_insn (gen_sse4_1_pinsrd (gen_rtx_SUBREG (V4SImode, vreg, 0),
> +                                       gen_rtx_SUBREG (V4SImode, vreg, 0),
> +                                       gen_rtx_SUBREG (SImode, reg, 4),
> +                                       GEN_INT (2)));
>         }
>        else
> -       emit_insn (gen_rtx_SET (gen_rtx_SUBREG (vmode, vreg, 0),
> -                               gen_gpr_to_xmm_move_src (vmode, reg)));
> -      rtx_insn *seq = get_insns ();
> -      end_sequence ();
> -      rtx_insn *insn = DF_REF_INSN (ref);
> -      emit_conversion_insns (seq, insn);
> -
> -      if (dump_file)
> -       fprintf (dump_file,
> -                "  Copied r%d to a vector register r%d for insn %d\n",
> -                regno, REGNO (vreg), INSN_UID (insn));
> +       {
> +         rtx tmp = gen_reg_rtx (DImode);
> +         emit_insn (gen_sse2_loadld (gen_rtx_SUBREG (V4SImode, vreg, 0),
> +                                     CONST0_RTX (V4SImode),
> +                                     gen_rtx_SUBREG (SImode, reg, 0)));
> +         emit_insn (gen_sse2_loadld (gen_rtx_SUBREG (V4SImode, tmp, 0),
> +                                     CONST0_RTX (V4SImode),
> +                                     gen_rtx_SUBREG (SImode, reg, 4)));
> +         emit_insn (gen_vec_interleave_lowv4si
> +                    (gen_rtx_SUBREG (V4SImode, vreg, 0),
> +                     gen_rtx_SUBREG (V4SImode, vreg, 0),
> +                     gen_rtx_SUBREG (V4SImode, tmp, 0)));
> +       }
>      }
> +  else
> +    emit_insn (gen_rtx_SET (gen_rtx_SUBREG (vmode, vreg, 0),
> +                           gen_gpr_to_xmm_move_src (vmode, reg)));
> +  rtx_insn *seq = get_insns ();
> +  end_sequence ();
> +  emit_conversion_insns (seq, insn);
> +
> +  if (dump_file)
> +    fprintf (dump_file,
> +            "  Copied r%d to a vector register r%d for insn %d\n",
> +            REGNO (reg), REGNO (vreg), INSN_UID (insn));
>  }
>
>  /* Copy the definition SRC of INSN inside the chain to DST for
> @@ -1158,7 +1164,11 @@ general_scalar_chain::convert_registers
>    bitmap_iterator bi;
>    unsigned id;
>    EXECUTE_IF_SET_IN_BITMAP (defs_conv, 0, id, bi)
> -    make_vector_copies (id);
> +    defs_map.put (regno_reg_rtx[id], gen_reg_rtx (smode));
> +  EXECUTE_IF_SET_IN_BITMAP (insns_conv, 0, id, bi)
> +    for (df_ref ref = DF_INSN_UID_DEFS (id); ref; ref = DF_REF_NEXT_LOC (ref))
> +      if (bitmap_bit_p (defs_conv, DF_REF_REGNO (ref)))
> +       make_vector_copies (DF_REF_INSN (ref), DF_REF_REAL_REG (ref));
>  }
>
>  /* Convert whole chain creating required register
> Index: gcc/config/i386/i386-features.h
> ===================================================================
> --- gcc/config/i386/i386-features.h     (revision 274945)
> +++ gcc/config/i386/i386-features.h     (working copy)
> @@ -167,16 +167,19 @@ class scalar_chain
>  class general_scalar_chain : public scalar_chain
>  {
>   public:
> -  general_scalar_chain (enum machine_mode smode_, enum machine_mode vmode_)
> -    : scalar_chain (smode_, vmode_) {}
> +  general_scalar_chain (enum machine_mode smode_, enum machine_mode vmode_);
> +  ~general_scalar_chain ();
>    int compute_convert_gain ();
>   private:
>    hash_map<rtx, rtx> defs_map;
> +  bitmap insns_conv;
> +  unsigned n_sse_to_integer;
> +  unsigned n_integer_to_sse;
>    void mark_dual_mode_def (df_ref def);
>    void convert_insn (rtx_insn *insn);
>    void convert_op (rtx *op, rtx_insn *insn);
>    void convert_reg (rtx_insn *insn, rtx dst, rtx src);
> -  void make_vector_copies (unsigned regno);
> +  void make_vector_copies (rtx_insn *, rtx);
>    void convert_registers ();
>    int vector_const_cost (rtx exp);
>  };


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]