Message ID | 1496324097-21221-7-git-send-email-claziss@synopsys.com |
---|---|
State | New |
Headers | show |
* Claudiu Zissulescu <Claudiu.Zissulescu@synopsys.com> [2017-06-01 15:34:56 +0200]: > From: claziss <claziss@synopsys.com> > > Emitting subregs in the expand is not a good idea. Deprecate this > option. > > gcc/ > 2017-04-26 Claudiu Zissulescu <claziss@synopsys.com> > > * config/arc/arc.md (adddi3): Remove support for mexpand-adddi > option. > (subdi3): Likewise. > * config/arc/arc.opt (mexpand-adddi): Deprecate it. > * doc/invoke.texi (mexpand-adddi): Update text. This looks fine, though the commit message tells me it's not a good idea, but it would be nice to know _why_ it's not good. Might be nice to know for future reference. Also, there's no test. Was there an issue that revealed this as not a good idea? Could that become a test? Thanks, Andrew > --- > gcc/config/arc/arc.md | 39 +-------------------------------------- > gcc/config/arc/arc.opt | 2 +- > gcc/doc/invoke.texi | 2 +- > 3 files changed, 3 insertions(+), 40 deletions(-) > > diff --git a/gcc/config/arc/arc.md b/gcc/config/arc/arc.md > index 928feb1..f595da7 100644 > --- a/gcc/config/arc/arc.md > +++ b/gcc/config/arc/arc.md > @@ -2649,30 +2649,7 @@ > (match_operand:DI 2 "nonmemory_operand" ""))) > (clobber (reg:CC CC_REG))])] > "" > -{ > - if (TARGET_EXPAND_ADDDI) > - { > - rtx l0 = gen_lowpart (SImode, operands[0]); > - rtx h0 = disi_highpart (operands[0]); > - rtx l1 = gen_lowpart (SImode, operands[1]); > - rtx h1 = disi_highpart (operands[1]); > - rtx l2 = gen_lowpart (SImode, operands[2]); > - rtx h2 = disi_highpart (operands[2]); > - rtx cc_c = gen_rtx_REG (CC_Cmode, CC_REG); > - > - if (CONST_INT_P (h2) && INTVAL (h2) < 0 && SIGNED_INT12 (INTVAL (h2))) > - { > - emit_insn (gen_sub_f (l0, l1, gen_int_mode (-INTVAL (l2), SImode))); > - emit_insn (gen_sbc (h0, h1, > - gen_int_mode (-INTVAL (h2) - (l1 != 0), SImode), > - cc_c)); > - DONE; > - } > - emit_insn (gen_add_f (l0, l1, l2)); > - emit_insn (gen_adc (h0, h1, h2)); > - DONE; > - } > -}) > +{}) > > ; This assumes that there can be no strictly partial overlap between > ; operands[1] and operands[2]. > @@ -2911,20 +2888,6 @@ > { > if (!register_operand (operands[2], DImode)) > operands[1] = force_reg (DImode, operands[1]); > - if (TARGET_EXPAND_ADDDI) > - { > - rtx l0 = gen_lowpart (SImode, operands[0]); > - rtx h0 = disi_highpart (operands[0]); > - rtx l1 = gen_lowpart (SImode, operands[1]); > - rtx h1 = disi_highpart (operands[1]); > - rtx l2 = gen_lowpart (SImode, operands[2]); > - rtx h2 = disi_highpart (operands[2]); > - rtx cc_c = gen_rtx_REG (CC_Cmode, CC_REG); > - > - emit_insn (gen_sub_f (l0, l1, l2)); > - emit_insn (gen_sbc (h0, h1, h2, cc_c)); > - DONE; > - } > }) > > (define_insn_and_split "subdi3_i" > diff --git a/gcc/config/arc/arc.opt b/gcc/config/arc/arc.opt > index ed2b827..ad2df26 100644 > --- a/gcc/config/arc/arc.opt > +++ b/gcc/config/arc/arc.opt > @@ -328,7 +328,7 @@ Target Var(TARGET_Q_CLASS) > Enable 'q' instruction alternatives. > > mexpand-adddi > -Target Var(TARGET_EXPAND_ADDDI) > +Target Warn(%qs is deprecated) > Expand adddi3 and subdi3 at rtl generation time into add.f / adc etc. > > > diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi > index 59563aa..b6cf4ce 100644 > --- a/gcc/doc/invoke.texi > +++ b/gcc/doc/invoke.texi > @@ -14823,7 +14823,7 @@ Enable pre-reload use of the @code{cbranchsi} pattern. > @item -mexpand-adddi > @opindex mexpand-adddi > Expand @code{adddi3} and @code{subdi3} at RTL generation time into > -@code{add.f}, @code{adc} etc. > +@code{add.f}, @code{adc} etc. This option is deprecated. > > @item -mindexed-loads > @opindex mindexed-loads > -- > 1.9.1 >
> > This looks fine, though the commit message tells me it's not a good > idea, but it would be nice to know _why_ it's not good. Might be nice > to know for future reference. > Again LRA, expand time subregs are not handled by LRA, as far as I can tell. Moreover, this option is by default off, and even using the old IRA may not be a good idea to have subregs before register allocation step. > Also, there's no test. Was there an issue that revealed this as not a > good idea? Could that become a test? > This error was found while running dg for our port with LRA on and this option on. Once removed the mexpand-adddi, a test is impossible to get. Thanks, Claudiu
> This looks fine, though the commit message tells me it's not a good > idea, but it would be nice to know _why_ it's not good. Might be nice > to know for future reference. Committed with additional comments. Thank you, Claudiu
diff --git a/gcc/config/arc/arc.md b/gcc/config/arc/arc.md index 928feb1..f595da7 100644 --- a/gcc/config/arc/arc.md +++ b/gcc/config/arc/arc.md @@ -2649,30 +2649,7 @@ (match_operand:DI 2 "nonmemory_operand" ""))) (clobber (reg:CC CC_REG))])] "" -{ - if (TARGET_EXPAND_ADDDI) - { - rtx l0 = gen_lowpart (SImode, operands[0]); - rtx h0 = disi_highpart (operands[0]); - rtx l1 = gen_lowpart (SImode, operands[1]); - rtx h1 = disi_highpart (operands[1]); - rtx l2 = gen_lowpart (SImode, operands[2]); - rtx h2 = disi_highpart (operands[2]); - rtx cc_c = gen_rtx_REG (CC_Cmode, CC_REG); - - if (CONST_INT_P (h2) && INTVAL (h2) < 0 && SIGNED_INT12 (INTVAL (h2))) - { - emit_insn (gen_sub_f (l0, l1, gen_int_mode (-INTVAL (l2), SImode))); - emit_insn (gen_sbc (h0, h1, - gen_int_mode (-INTVAL (h2) - (l1 != 0), SImode), - cc_c)); - DONE; - } - emit_insn (gen_add_f (l0, l1, l2)); - emit_insn (gen_adc (h0, h1, h2)); - DONE; - } -}) +{}) ; This assumes that there can be no strictly partial overlap between ; operands[1] and operands[2]. @@ -2911,20 +2888,6 @@ { if (!register_operand (operands[2], DImode)) operands[1] = force_reg (DImode, operands[1]); - if (TARGET_EXPAND_ADDDI) - { - rtx l0 = gen_lowpart (SImode, operands[0]); - rtx h0 = disi_highpart (operands[0]); - rtx l1 = gen_lowpart (SImode, operands[1]); - rtx h1 = disi_highpart (operands[1]); - rtx l2 = gen_lowpart (SImode, operands[2]); - rtx h2 = disi_highpart (operands[2]); - rtx cc_c = gen_rtx_REG (CC_Cmode, CC_REG); - - emit_insn (gen_sub_f (l0, l1, l2)); - emit_insn (gen_sbc (h0, h1, h2, cc_c)); - DONE; - } }) (define_insn_and_split "subdi3_i" diff --git a/gcc/config/arc/arc.opt b/gcc/config/arc/arc.opt index ed2b827..ad2df26 100644 --- a/gcc/config/arc/arc.opt +++ b/gcc/config/arc/arc.opt @@ -328,7 +328,7 @@ Target Var(TARGET_Q_CLASS) Enable 'q' instruction alternatives. mexpand-adddi -Target Var(TARGET_EXPAND_ADDDI) +Target Warn(%qs is deprecated) Expand adddi3 and subdi3 at rtl generation time into add.f / adc etc. diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index 59563aa..b6cf4ce 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -14823,7 +14823,7 @@ Enable pre-reload use of the @code{cbranchsi} pattern. @item -mexpand-adddi @opindex mexpand-adddi Expand @code{adddi3} and @code{subdi3} at RTL generation time into -@code{add.f}, @code{adc} etc. +@code{add.f}, @code{adc} etc. This option is deprecated. @item -mindexed-loads @opindex mindexed-loads
From: claziss <claziss@synopsys.com> Emitting subregs in the expand is not a good idea. Deprecate this option. gcc/ 2017-04-26 Claudiu Zissulescu <claziss@synopsys.com> * config/arc/arc.md (adddi3): Remove support for mexpand-adddi option. (subdi3): Likewise. * config/arc/arc.opt (mexpand-adddi): Deprecate it. * doc/invoke.texi (mexpand-adddi): Update text. --- gcc/config/arc/arc.md | 39 +-------------------------------------- gcc/config/arc/arc.opt | 2 +- gcc/doc/invoke.texi | 2 +- 3 files changed, 3 insertions(+), 40 deletions(-)