This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [PATCH] Fix PR69771, bogus CONST_INT during shift expansion
- From: Richard Biener <rguenther at suse dot de>
- To: Jakub Jelinek <jakub at redhat dot com>,James Greenhalgh <james dot greenhalgh at arm dot com>
- Cc: Bernd Schmidt <bschmidt at redhat dot com>,gcc-patches at gcc dot gnu dot org
- Date: Mon, 15 Feb 2016 18:58:45 +0100
- Subject: Re: [PATCH] Fix PR69771, bogus CONST_INT during shift expansion
- Authentication-results: sourceware.org; auth=none
- References: <alpine dot LSU dot 2 dot 11 dot 1602121145370 dot 31122 at t29 dot fhfr dot qr> <56BDCCCF dot 2030601 at redhat dot com> <20160212122438 dot GP3017 at tucnak dot redhat dot com> <alpine dot LSU dot 2 dot 11 dot 1602121346100 dot 31122 at t29 dot fhfr dot qr> <alpine dot LSU dot 2 dot 11 dot 1602121349340 dot 31122 at t29 dot fhfr dot qr> <20160212132813 dot GR3017 at tucnak dot redhat dot com> <alpine dot LSU dot 2 dot 11 dot 1602121446430 dot 31122 at t29 dot fhfr dot qr> <56BDEA17 dot 7090808 at redhat dot com> <20160212163421 dot GT3017 at tucnak dot redhat dot com> <20160213075024 dot GA17920 at arm dot com> <20160215153438 dot GA3017 at tucnak dot redhat dot com>
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
- References:
- Re: [PATCH] Fix PR69771, bogus CONST_INT during shift expansion
- Re: [PATCH] Fix PR69771, bogus CONST_INT during shift expansion
- Re: [PATCH] Fix PR69771, bogus CONST_INT during shift expansion
- Re: [PATCH] Fix PR69771, bogus CONST_INT during shift expansion
- Re: [PATCH] Fix PR69771, bogus CONST_INT during shift expansion
- Re: [PATCH] Fix PR69771, bogus CONST_INT during shift expansion
- Re: [PATCH] Fix PR69771, bogus CONST_INT during shift expansion
- Re: [PATCH] Fix PR69771, bogus CONST_INT during shift expansion
- Re: [PATCH] Fix PR69771, bogus CONST_INT during shift expansion
- Re: [PATCH] Fix PR69771, bogus CONST_INT during shift expansion
- Re: [PATCH] Fix PR69771, bogus CONST_INT during shift expansion