This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] PR target/71549: Convert V1TImode register to TImode in debug insn
- From: "H.J. Lu" <hjl dot tools at gmail dot com>
- To: Ilya Enkovich <enkovich dot gnu at gmail dot com>
- Cc: Uros Bizjak <ubizjak at gmail dot com>, "gcc-patches at gcc dot gnu dot org" <gcc-patches at gcc dot gnu dot org>
- Date: Mon, 20 Jun 2016 11:53:48 -0700
- Subject: Re: [PATCH] PR target/71549: Convert V1TImode register to TImode in debug insn
- Authentication-results: sourceware.org; auth=none
- References: <20160620115515 dot GA4587 at intel dot com> <CAFULd4Yx284y=UggLcvj4ZSB-RPzSys-5xV5Q5GCzRornMHE0Q at mail dot gmail dot com> <CAMbmDYZjc6i_j2gBV4APhUErbS4N0w7iOfsOAMGAOcYUWjwnAw at mail dot gmail dot com> <CAMe9rOq3=sMc5wPzmo2ker1yFDXF2BdmYmtZUgg5jF_H2Pfq9g at mail dot gmail dot com> <20160620173130 dot GA30064 at msticlxl57 dot ims dot intel dot com>
On Mon, Jun 20, 2016 at 10:31 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
> On 20 Jun 09:45, H.J. Lu wrote:
>> On Mon, Jun 20, 2016 at 7:30 AM, Ilya Enkovich <enkovich.gnu@gmail.com> wrote:
>> > 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).
>>
>> Yes, it can happen:
>>
>> (insn 11 8 12 2 (parallel [
>> (set (reg/v:TI 91 [ <retval> ])
>> (plus:TI (reg/v:TI 92 [ a ])
>> (reg/v:TI 96 [ b ])))
>> (clobber (reg:CC 17 flags))
>> ]) y.i:5 210 {*addti3_doubleword}
>> (expr_list:REG_UNUSED (reg:CC 17 flags)
>> (nil)))
>> (debug_insn 12 11 13 2 (var_location:TI w (reg/v:TI 91 [ <retval> ])) y.i:5 -1
>> (nil))
>>
>>
>> > 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?
>>
>> Debug insn has no impact on STV decision. We just need to convert
>> register referenced in debug insn from V1TImode to TImode in
>> timode_scalar_chain::convert_insn.
>>
>> > 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.
>>
>> Here is the updated patch to add debug insn, which references the
>> TImode register which will be converted to V1TImode to queue.
>> I am testing it now.
>>
>
> You still count and dump debug insns as optimized ones. Also we
> try to use virtual functions to cover differences in DI and TI
> optimizations and introducing additional TARGET_64BIT in common
> STV code is undesirable.
>
> Also your conversion now depends on instructions processing order.
> You will fail to process debug insn before non-debug ones. Required
> order is not guaranteed because processing depends on instruction
> UIDs only.
>
> I propose to modify transformation phase only like in the patch
> (untested) below. I rely on your code which assumes the only
> possible usage in debug insn is VAR_LOCATION.
>
> Thanks,
> Ilya
> --
> diff --git a/gcc/config/i386/i386.c b/gcc/config/i386/i386.c
> index c5e5e12..ec955f0 100644
> --- a/gcc/config/i386/i386.c
> +++ b/gcc/config/i386/i386.c
> @@ -3139,6 +3139,7 @@ class timode_scalar_chain : public scalar_chain
>
> private:
> void mark_dual_mode_def (df_ref def);
> + void fix_debug_reg_uses (rtx reg);
> void convert_insn (rtx_insn *insn);
> /* We don't convert registers to difference size. */
> void convert_registers () {}
> @@ -3790,6 +3791,34 @@ dimode_scalar_chain::convert_insn (rtx_insn *insn)
> df_insn_rescan (insn);
> }
>
> +/* Fix uses of converted REG in debug insns. */
> +
> +void
> +timode_scalar_chain::fix_debug_reg_uses (rtx reg)
> +{
> + df_ref ref;
> + for (ref = DF_REG_USE_CHAIN (REGNO (reg)); ref; ref = DF_REF_NEXT_REG (ref))
> + {
> + rtx_insn *insn = DF_REF_INSN (ref);
> +
> + 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)
> + && GET_MODE (loc) == V1TImode);
> + /* Convert V1TImode register, which has been updated by a SET
> + insn before, to SUBREG TImode. */
> + PAT_VAR_LOCATION_LOC (val) = gen_rtx_SUBREG (TImode, loc, 0);
> + df_insn_rescan (insn);
> + return;
> + }
> + }
> +}
> +
> /* Convert INSN from TImode to V1T1mode. */
>
> void
> @@ -3806,8 +3835,10 @@ timode_scalar_chain::convert_insn (rtx_insn *insn)
> rtx tmp = find_reg_equal_equiv_note (insn);
> if (tmp)
> PUT_MODE (XEXP (tmp, 0), V1TImode);
> + PUT_MODE (dst, V1TImode);
> + fix_debug_reg_uses (dst);
> }
> - /* FALLTHRU */
> + break;
> case MEM:
> PUT_MODE (dst, V1TImode);
> break;
Won't be it waste CPU cycles when there is no debug insin
which use TImode registers?
--
H.J.