| Message ID | 20160215153438.GA3017@tucnak.redhat.com |
|---|---|
| State | New |
| Headers | show |
On February 15, 2016 4:34:38 PM GMT+01:00, Jakub Jelinek <jakub@redhat.com> wrote: >On Sat, Feb 13, 2016 at 07:50:25AM +0000, James Greenhalgh wrote: >> On Fri, Feb 12, 2016 at 05:34:21PM +0100, Jakub Jelinek wrote: >> > On Fri, Feb 12, 2016 at 03:20:07PM +0100, Bernd Schmidt wrote: >> > > >>- mode1 = GET_MODE (xop1) != VOIDmode ? GET_MODE (xop1) : >mode; >> > > >>+ mode1 = GET_MODE (xop1) != VOIDmode ? GET_MODE (xop1) : >mode1; >> > > >> if (xmode1 != VOIDmode && xmode1 != mode1) >> > > >> { >> > > >> > > Here, things aren't so clear, and the fact that the mode1 >calculation now >> > > differs from the mode0 one may be overlooked by someone in the >future. >> > > >> > > Rather than use codes like "mode variable is VOIDmode", I'd >prefer to use >> > > booleans with descriptive names, like "op1_may_need_conversion". >> > >> > So do you prefer e.g. following? Bootstrapped/regtested on >x86_64-linux and >> > i686-linux. >> > >> > 2016-02-12 Jakub Jelinek <jakub@redhat.com> >> > >> > PR rtl-optimization/69764 >> > PR rtl-optimization/69771 >> > * optabs.c (expand_binop_directly): For shift_optab_p, force >> > convert_modes with VOIDmode if xop1 has VOIDmode. >> > >> > * c-c++-common/pr69764.c: New test. >> > * gcc.dg/torture/pr69771.c: New testcase. >> > >> >> These two new tests are failing for me on AArch64 as so: > >As I said earlier, I wanted to fix it in expand_binop_directly because >the >higher levels still GEN_INT the various shift counters and then call >expand_binop_directly. But, as can be seen on aarch64/arm/m68k, there >are >cases that need op1 to be valid for mode already in expand_binop, so in >addition to the already committed fix I think we need to handle it >at the expand_binop level too. >As we don't have a single spot with convert_modes like >expand_binop_directly, I think the best is to do there a change >for the uncommon and invalid cases only, like (seems to fix the ICE >both on aarch64 and m68k): We could also force_reg those at expansion or apply SHIFT_COUNT_TRUNCATED to those invalid constants there. Richard. >2016-02-15 Jakub Jelinek <jakub@redhat.com> > > PR rtl-optimization/69764 > PR rtl-optimization/69771 > * optabs.c (expand_binop): Ensure for shift optabs invalid CONST_INT > op1 is valid for mode. > >--- gcc/optabs.c.jj 2016-02-12 17:49:25.000000000 +0100 >+++ gcc/optabs.c 2016-02-15 16:15:53.983673792 +0100 >@@ -1125,6 +1125,12 @@ expand_binop (machine_mode mode, optab b > op1 = negate_rtx (mode, op1); > binoptab = add_optab; > } >+ /* For shifts, constant invalid op1 might be expanded from different >+ mode than MODE. */ >+ else if (CONST_INT_P (op1) >+ && shift_optab_p (binoptab) >+ && UINTVAL (op1) >= GET_MODE_BITSIZE (GET_MODE_INNER (mode))) >+ op1 = gen_int_mode (INTVAL (op1), mode); > > /* Record where to delete back to if we backtrack. */ > last = get_last_insn (); > > > Jakub
On Mon, Feb 15, 2016 at 06:58:45PM +0100, Richard Biener wrote: > We could also force_reg those at expansion or apply SHIFT_COUNT_TRUNCATED to those invalid constants there. Sure, but for force_reg we'd still need the gen_int_mode anyway. As for SHIFT_COUNT_TRUNCATED, it should have been applied already from the caller - expand_shift_1. > >2016-02-15 Jakub Jelinek <jakub@redhat.com> > > > > PR rtl-optimization/69764 > > PR rtl-optimization/69771 > > * optabs.c (expand_binop): Ensure for shift optabs invalid CONST_INT > > op1 is valid for mode. > > > >--- gcc/optabs.c.jj 2016-02-12 17:49:25.000000000 +0100 > >+++ gcc/optabs.c 2016-02-15 16:15:53.983673792 +0100 > >@@ -1125,6 +1125,12 @@ expand_binop (machine_mode mode, optab b > > op1 = negate_rtx (mode, op1); > > binoptab = add_optab; > > } > >+ /* For shifts, constant invalid op1 might be expanded from different > >+ mode than MODE. */ > >+ else if (CONST_INT_P (op1) > >+ && shift_optab_p (binoptab) > >+ && UINTVAL (op1) >= GET_MODE_BITSIZE (GET_MODE_INNER (mode))) > >+ op1 = gen_int_mode (INTVAL (op1), mode); > > > > /* Record where to delete back to if we backtrack. */ > > last = get_last_insn (); Jakub
On February 15, 2016 7:15:35 PM GMT+01:00, Jakub Jelinek <jakub@redhat.com> wrote: >On Mon, Feb 15, 2016 at 06:58:45PM +0100, Richard Biener wrote: >> We could also force_reg those at expansion or apply >SHIFT_COUNT_TRUNCATED to those invalid constants there. > >Sure, but for force_reg we'd still need the gen_int_mode anyway. >As for SHIFT_COUNT_TRUNCATED, it should have been applied already from >the >caller - expand_shift_1. But then no out of bound values should remain. Until we get 256bit ints where your workaround wouldn't work either? Richard. >> >2016-02-15 Jakub Jelinek <jakub@redhat.com> >> > >> > PR rtl-optimization/69764 >> > PR rtl-optimization/69771 >> > * optabs.c (expand_binop): Ensure for shift optabs invalid >CONST_INT >> > op1 is valid for mode. >> > >> >--- gcc/optabs.c.jj 2016-02-12 17:49:25.000000000 +0100 >> >+++ gcc/optabs.c 2016-02-15 16:15:53.983673792 +0100 >> >@@ -1125,6 +1125,12 @@ expand_binop (machine_mode mode, optab b >> > op1 = negate_rtx (mode, op1); >> > binoptab = add_optab; >> > } >> >+ /* For shifts, constant invalid op1 might be expanded from >different >> >+ mode than MODE. */ >> >+ else if (CONST_INT_P (op1) >> >+ && shift_optab_p (binoptab) >> >+ && UINTVAL (op1) >= GET_MODE_BITSIZE (GET_MODE_INNER (mode))) >> >+ op1 = gen_int_mode (INTVAL (op1), mode); >> > >> > /* Record where to delete back to if we backtrack. */ >> > last = get_last_insn (); > > Jakub
--- gcc/optabs.c.jj 2016-02-12 17:49:25.000000000 +0100 +++ gcc/optabs.c 2016-02-15 16:15:53.983673792 +0100 @@ -1125,6 +1125,12 @@ expand_binop (machine_mode mode, optab b op1 = negate_rtx (mode, op1); binoptab = add_optab; } + /* For shifts, constant invalid op1 might be expanded from different + mode than MODE. */ + else if (CONST_INT_P (op1) + && shift_optab_p (binoptab) + && UINTVAL (op1) >= GET_MODE_BITSIZE (GET_MODE_INNER (mode))) + op1 = gen_int_mode (INTVAL (op1), mode); /* Record where to delete back to if we backtrack. */ last = get_last_insn ();