Re: [PATCH] PR target/71549: Convert V1TImode register to TImode in debug insn

[Ilya Enkovich <enkovich dot gnu at gmail dot com>]

2016-06-20 16:39 GMT+03:00 Uros Bizjak <ubizjak@gmail.com>:
> On Mon, Jun 20, 2016 at 1:55 PM, H.J. Lu <hongjiu.lu@intel.com> wrote:
>> TImode register referenced in debug insn can be converted to V1TImode
>> by scalar to vector optimization.  We need to convert a debug insn if
>> it has a variable in a TImode register.
>>
>> Tested on x86-64.  OK for trunk?
>
> Ilya, does this approach look good to you? Also, does DImode STV
> conversion need similar handling of debug insns?

DImode conversion doesn't change register mode (i.e. never calls
PUT_MODE for registers).  That keeps debug instructions valid.

Overall I don't like the idea of having debug insns in candidates
set and in chains.  Looks like it is possible to have a chain
consisting of a debug insn only which is weird (otherwise I don't
see why we may return false in timode_scalar_chain::convert_insn).

What about other possible register uses?  If debug insns are added
to candidates then NONDEBUG_INSN_P check for uses in
timode_check_non_convertible_regs becomes invalid, right?

If we have (or want) to fix some register uses then it's probably
would be better to visit register uses when we convert its mode
and make required fix-ups.  It seems better to me to not involve
debug insns in analysis phase.

Also I don't think debug insns should be accounted as optimized
instructions because we would get different number of optimized
instructions depending on debug info availability which may be
inconvenient for dump scans (and it is not a real instruction
optimization).

Thanks,
Ilya

>
> Uros.
>
>>
>> H.J.
>> ----
>> gcc/
>>
>>         PR target/71549
>>         * config/i386/i386.c (timode_scalar_to_vector_candidate_p):
>>         Return true if debug insn has a variable in TImode register.
>>         (timode_remove_non_convertible_regs): Skip debug insn.
>>         (scalar_chain::convert_insn): Change return type to bool.
>>         (scalar_chain::add_insn): Don't check registers in debug insn.
>>         (dimode_scalar_chain::convert_insn): Change return type to bool
>>         and always return true.
>>         (timode_scalar_chain::convert_insn): Change return type to bool.
>>         Convert V1TImode register to SUBREG TImode in debug insn.  Return
>>         false if debug insn isn't converted.  Otherwise, return true.
>>         (scalar_chain::convert): Increment converted_insns only if
>>         convert_insn returns true.
>>
>> gcc/testsuite/
>>
>>         PR target/71549
>>         * gcc.target/i386/pr71549.c: New test.
>> ---
>>  gcc/config/i386/i386.c                  | 58 ++++++++++++++++++++++++++++-----
>>  gcc/testsuite/gcc.target/i386/pr71549.c | 24 ++++++++++++++
>>  2 files changed, 74 insertions(+), 8 deletions(-)
>>  create mode 100644 gcc/testsuite/gcc.target/i386/pr71549.c
>>
>> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
>> index 56a5b9c..e17fc53 100644
>> --- a/gcc/config/i386/i386.c
>> +++ b/gcc/config/i386/i386.c
>> @@ -2845,6 +2845,16 @@ dimode_scalar_to_vector_candidate_p (rtx_insn *insn)
>>  static bool
>>  timode_scalar_to_vector_candidate_p (rtx_insn *insn)
>>  {
>> +  if (DEBUG_INSN_P (insn))
>> +    {
>> +      /* If a variable is put in a TImode register, which may be
>> +        converted to V1TImode, we need to convert this debug insn.  */
>> +      rtx val = PATTERN (insn);
>> +      return (GET_MODE (val) == TImode
>> +             && GET_CODE (val) == VAR_LOCATION
>> +             && REG_P (PAT_VAR_LOCATION_LOC (val)));
>> +    }
>> +
>>    rtx def_set = single_set (insn);
>>
>>    if (!def_set)
>> @@ -3012,7 +3022,12 @@ timode_remove_non_convertible_regs (bitmap candidates)
>>
>>    EXECUTE_IF_SET_IN_BITMAP (candidates, 0, id, bi)
>>      {
>> -      rtx def_set = single_set (DF_INSN_UID_GET (id)->insn);
>> +      rtx_insn *insn = DF_INSN_UID_GET (id)->insn;
>> +      /* Debug insn isn't a SET insn.  */
>> +      if (DEBUG_INSN_P (insn))
>> +       continue;
>> +
>> +      rtx def_set = single_set (insn);
>>        rtx dest = SET_DEST (def_set);
>>        rtx src = SET_SRC (def_set);
>>
>> @@ -3111,7 +3126,7 @@ class scalar_chain
>>    void add_insn (bitmap candidates, unsigned insn_uid);
>>    void analyze_register_chain (bitmap candidates, df_ref ref);
>>    virtual void mark_dual_mode_def (df_ref def) = 0;
>> -  virtual void convert_insn (rtx_insn *insn) = 0;
>> +  virtual bool convert_insn (rtx_insn *insn) = 0;
>>    virtual void convert_registers () = 0;
>>  };
>>
>> @@ -3123,7 +3138,7 @@ class dimode_scalar_chain : public scalar_chain
>>    void mark_dual_mode_def (df_ref def);
>>    rtx replace_with_subreg (rtx x, rtx reg, rtx subreg);
>>    void replace_with_subreg_in_insn (rtx_insn *insn, rtx reg, rtx subreg);
>> -  void convert_insn (rtx_insn *insn);
>> +  bool convert_insn (rtx_insn *insn);
>>    void convert_op (rtx *op, rtx_insn *insn);
>>    void convert_reg (unsigned regno);
>>    void make_vector_copies (unsigned regno);
>> @@ -3139,7 +3154,7 @@ class timode_scalar_chain : public scalar_chain
>>
>>   private:
>>    void mark_dual_mode_def (df_ref def);
>> -  void convert_insn (rtx_insn *insn);
>> +  bool convert_insn (rtx_insn *insn);
>>    /* We don't convert registers to difference size.  */
>>    void convert_registers () {}
>>  };
>> @@ -3276,6 +3291,10 @@ scalar_chain::add_insn (bitmap candidates, unsigned int insn_uid)
>>    bitmap_set_bit (insns, insn_uid);
>>
>>    rtx_insn *insn = DF_INSN_UID_GET (insn_uid)->insn;
>> +  /* Debug insn isn't a SET insn.  */
>> +  if (DEBUG_INSN_P (insn))
>> +    return;
>> +
>>    rtx def_set = single_set (insn);
>>    if (def_set && REG_P (SET_DEST (def_set))
>>        && !HARD_REGISTER_P (SET_DEST (def_set)))
>> @@ -3708,7 +3727,7 @@ dimode_scalar_chain::convert_op (rtx *op, rtx_insn *insn)
>>
>>  /* Convert INSN to vector mode.  */
>>
>> -void
>> +bool
>>  dimode_scalar_chain::convert_insn (rtx_insn *insn)
>>  {
>>    rtx def_set = single_set (insn);
>> @@ -3788,13 +3807,34 @@ dimode_scalar_chain::convert_insn (rtx_insn *insn)
>>    INSN_CODE (insn) = -1;
>>    recog_memoized (insn);
>>    df_insn_rescan (insn);
>> +
>> +  return true;
>>  }
>>
>>  /* Convert INSN from TImode to V1T1mode.  */
>>
>> -void
>> +bool
>>  timode_scalar_chain::convert_insn (rtx_insn *insn)
>>  {
>> +  if (DEBUG_INSN_P (insn))
>> +    {
>> +      /* It must be a debug insn with a TImode variable in register.  */
>> +      rtx val = PATTERN (insn);
>> +      gcc_assert (GET_MODE (val) == TImode
>> +                 && GET_CODE (val) == VAR_LOCATION);
>> +      rtx loc = PAT_VAR_LOCATION_LOC (val);
>> +      gcc_assert (REG_P (loc));
>> +      /* Convert V1TImode register, which has been updated by a SET
>> +        insn before, to SUBREG TImode.  */
>> +      if (GET_MODE (loc) == V1TImode)
>> +       {
>> +         PAT_VAR_LOCATION_LOC (val) = gen_rtx_SUBREG (TImode, loc, 0);
>> +         df_insn_rescan (insn);
>> +         return true;
>> +       }
>> +      return false;
>> +    }
>> +
>>    rtx def_set = single_set (insn);
>>    rtx src = SET_SRC (def_set);
>>    rtx dst = SET_DEST (def_set);
>> @@ -3858,6 +3898,8 @@ timode_scalar_chain::convert_insn (rtx_insn *insn)
>>    INSN_CODE (insn) = -1;
>>    recog_memoized (insn);
>>    df_insn_rescan (insn);
>> +
>> +  return true;
>>  }
>>
>>  void
>> @@ -3893,8 +3935,8 @@ scalar_chain::convert ()
>>
>>    EXECUTE_IF_SET_IN_BITMAP (insns, 0, id, bi)
>>      {
>> -      convert_insn (DF_INSN_UID_GET (id)->insn);
>> -      converted_insns++;
>> +      if (convert_insn (DF_INSN_UID_GET (id)->insn))
>> +       converted_insns++;
>>      }
>>
>>    return converted_insns;
>> diff --git a/gcc/testsuite/gcc.target/i386/pr71549.c b/gcc/testsuite/gcc.target/i386/pr71549.c
>> new file mode 100644
>> index 0000000..8aac891
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/i386/pr71549.c
>> @@ -0,0 +1,24 @@
>> +/* { dg-do compile } */
>> +/* { dg-options "-O2 -g" } */
>> +
>> +struct S1
>> +{
>> +  int f0;
>> +  int f1;
>> +  int f2;
>> +  int:4;
>> +} a, b;
>> +
>> +void
>> +fn1 (struct S1 p1)
>> +{
>> +  a = p1;
>> +  int c = p1.f0;
>> +}
>> +
>> +int
>> +main ()
>> +{
>> +  fn1 (b);
>> +  return 0;
>> +}
>> --
>> 2.5.5
>>