| Message ID | 20160620173130.GA30064@msticlxl57.ims.intel.com |
|---|---|
| State | New |
| Headers | show |
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?
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;