[PATCH][GCC 7] Remove broken path in extract_bit_field_1

[PATCH][GCC 7] Remove broken path in extract_bit_field_1

[Richard Biener]

r141430 as a fix for PR37870 added a memory fallback to 
extract_bit_field_1 and another case punning through a larger integer
mode (as suggested by Ian).  That path is broken as it happily creates

(set (subreg:XF (reg:TI ..)) (...))

on i?86 when we optimize the added testcase

unsigned int
bar (long double x)
{
  union { struct { char a[8]; unsigned int b:7; } c; long double d; } u;
  u.d = x;
  return u.c.b;
}

on GIMPLE to

bar (long double x)
{
  unsigned int _3;
  <unnamed-unsigned:7> _4;

;;   basic block 2, loop depth 0
;;    pred:       ENTRY
  _4 = BIT_FIELD_REF <x_2(D), 7, 64>;
  _3 = (unsigned int) _4;
  return _3;


Bootstrapped and tested on x86_64-unknown-linux-gnu (also with -m32)
and the code replaced with a gcc_unreachable ().

I don't see how a subreg can be ever valid as it will be necessarily
converting to a non-integer mode of different size (otherwise 
int_mode_for_mode would have returned non-BLKmode).  I can't think
of a condition that would need to be satisfied where the subreg
would be valid either.

Ok for GCC 7?  (it'll be done as part of said GIMPLE optimization).

The testcase gcc.target/i386/pr37870.c will already ICE with that
patch, so no additional testcase.

Thanks,
Richard.

2016-04-04  Richard Biener  <rguenther@suse.de>

	PR middle-end/37870
	* expmed.c (extract_bit_field_1): Remove broken case
	using a wider MODE_INT mode.

Index: gcc/expmed.c
===================================================================
--- gcc/expmed.c	(revision 234708)
+++ gcc/expmed.c	(working copy)
@@ -1647,17 +1647,6 @@ extract_bit_field_1 (rtx str_rtx, unsign
 	    if (GET_CODE (op0) == SUBREG)
 	      op0 = force_reg (imode, op0);
 	  }
-	else if (REG_P (op0))
-	  {
-	    rtx reg, subreg;
-	    imode = smallest_mode_for_size (GET_MODE_BITSIZE (GET_MODE (op0)),
-					    MODE_INT);
-	    reg = gen_reg_rtx (imode);
-	    subreg = gen_lowpart_SUBREG (GET_MODE (op0), reg);
-	    emit_move_insn (subreg, op0);
-	    op0 = reg;
-	    bitnum += SUBREG_BYTE (subreg) * BITS_PER_UNIT;
-	  }
 	else
 	  {
 	    HOST_WIDE_INT size = GET_MODE_SIZE (GET_MODE (op0));

Re: [PATCH][GCC 7] Remove broken path in extract_bit_field_1

[Jakub Jelinek]

On Mon, Apr 04, 2016 at 02:56:51PM +0200, Richard Biener wrote:
> The testcase gcc.target/i386/pr37870.c will already ICE with that
> patch, so no additional testcase.

In theory you could validate_subreg first and use that code if validation
went ok, otherwise go through memory.
But I admit I don't have anything in particular in mind where it would
trigger this code and the subreg would successfully validate.

> 2016-04-04  Richard Biener  <rguenther@suse.de>
> 
> 	PR middle-end/37870
> 	* expmed.c (extract_bit_field_1): Remove broken case
> 	using a wider MODE_INT mode.
> 
> Index: gcc/expmed.c
> ===================================================================
> --- gcc/expmed.c	(revision 234708)
> +++ gcc/expmed.c	(working copy)
> @@ -1647,17 +1647,6 @@ extract_bit_field_1 (rtx str_rtx, unsign
>  	    if (GET_CODE (op0) == SUBREG)
>  	      op0 = force_reg (imode, op0);
>  	  }
> -	else if (REG_P (op0))
> -	  {
> -	    rtx reg, subreg;
> -	    imode = smallest_mode_for_size (GET_MODE_BITSIZE (GET_MODE (op0)),
> -					    MODE_INT);
> -	    reg = gen_reg_rtx (imode);
> -	    subreg = gen_lowpart_SUBREG (GET_MODE (op0), reg);
> -	    emit_move_insn (subreg, op0);
> -	    op0 = reg;
> -	    bitnum += SUBREG_BYTE (subreg) * BITS_PER_UNIT;
> -	  }
>  	else
>  	  {
>  	    HOST_WIDE_INT size = GET_MODE_SIZE (GET_MODE (op0));

	Jakub

Re: [PATCH][GCC 7] Remove broken path in extract_bit_field_1

[Richard Biener]

On Mon, 4 Apr 2016, Jakub Jelinek wrote:

> On Mon, Apr 04, 2016 at 02:56:51PM +0200, Richard Biener wrote:
> > The testcase gcc.target/i386/pr37870.c will already ICE with that
> > patch, so no additional testcase.
> 
> In theory you could validate_subreg first and use that code if validation
> went ok, otherwise go through memory.
> But I admit I don't have anything in particular in mind where it would
> trigger this code and the subreg would successfully validate.

Not sure if it would help as that has

  /* ??? Similarly, e.g. with (subreg:DF (reg:TI)).  Though 
store_bit_field
     is the culprit here, and not the backends.  */
  else if (osize >= UNITS_PER_WORD && isize >= osize)
    ;

and thus we'd return true anyway for (subreg:XF (reg:TI) 0)

Richard.

> > 2016-04-04  Richard Biener  <rguenther@suse.de>
> > 
> > 	PR middle-end/37870
> > 	* expmed.c (extract_bit_field_1): Remove broken case
> > 	using a wider MODE_INT mode.
> > 
> > Index: gcc/expmed.c
> > ===================================================================
> > --- gcc/expmed.c	(revision 234708)
> > +++ gcc/expmed.c	(working copy)
> > @@ -1647,17 +1647,6 @@ extract_bit_field_1 (rtx str_rtx, unsign
> >  	    if (GET_CODE (op0) == SUBREG)
> >  	      op0 = force_reg (imode, op0);
> >  	  }
> > -	else if (REG_P (op0))
> > -	  {
> > -	    rtx reg, subreg;
> > -	    imode = smallest_mode_for_size (GET_MODE_BITSIZE (GET_MODE (op0)),
> > -					    MODE_INT);
> > -	    reg = gen_reg_rtx (imode);
> > -	    subreg = gen_lowpart_SUBREG (GET_MODE (op0), reg);
> > -	    emit_move_insn (subreg, op0);
> > -	    op0 = reg;
> > -	    bitnum += SUBREG_BYTE (subreg) * BITS_PER_UNIT;
> > -	  }
> >  	else
> >  	  {
> >  	    HOST_WIDE_INT size = GET_MODE_SIZE (GET_MODE (op0));
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)

Re: [PATCH][GCC 7] Remove broken path in extract_bit_field_1

[Jakub Jelinek]

On Mon, Apr 04, 2016 at 03:10:34PM +0200, Richard Biener wrote:
> On Mon, 4 Apr 2016, Jakub Jelinek wrote:
> 
> > On Mon, Apr 04, 2016 at 02:56:51PM +0200, Richard Biener wrote:
> > > The testcase gcc.target/i386/pr37870.c will already ICE with that
> > > patch, so no additional testcase.
> > 
> > In theory you could validate_subreg first and use that code if validation
> > went ok, otherwise go through memory.
> > But I admit I don't have anything in particular in mind where it would
> > trigger this code and the subreg would successfully validate.
> 
> Not sure if it would help as that has
> 
>   /* ??? Similarly, e.g. with (subreg:DF (reg:TI)).  Though 
> store_bit_field
>      is the culprit here, and not the backends.  */
>   else if (osize >= UNITS_PER_WORD && isize >= osize)
>     ;
> 
> and thus we'd return true anyway for (subreg:XF (reg:TI) 0)

If XFmode subreg of TImode reg passes validation, where does it ICE then?

	Jakub

Re: [PATCH][GCC 7] Remove broken path in extract_bit_field_1

[Richard Biener]

On Mon, 4 Apr 2016, Jakub Jelinek wrote:

> On Mon, Apr 04, 2016 at 03:10:34PM +0200, Richard Biener wrote:
> > On Mon, 4 Apr 2016, Jakub Jelinek wrote:
> > 
> > > On Mon, Apr 04, 2016 at 02:56:51PM +0200, Richard Biener wrote:
> > > > The testcase gcc.target/i386/pr37870.c will already ICE with that
> > > > patch, so no additional testcase.
> > > 
> > > In theory you could validate_subreg first and use that code if validation
> > > went ok, otherwise go through memory.
> > > But I admit I don't have anything in particular in mind where it would
> > > trigger this code and the subreg would successfully validate.
> > 
> > Not sure if it would help as that has
> > 
> >   /* ??? Similarly, e.g. with (subreg:DF (reg:TI)).  Though 
> > store_bit_field
> >      is the culprit here, and not the backends.  */
> >   else if (osize >= UNITS_PER_WORD && isize >= osize)
> >     ;
> > 
> > and thus we'd return true anyway for (subreg:XF (reg:TI) 0)
> 
> If XFmode subreg of TImode reg passes validation, where does it ICE then?

It ICEs in

/space/rguenther/src/svn/trunk/gcc/testsuite/gcc.target/i386/pr37870.c:19:1: 
internal compiler error: in subreg_get_info, at rtlanal.c:3695
0xddee5a subreg_get_info(unsigned int, machine_mode, unsigned int, 
machine_mode, subreg_info*)
        /space/rguenther/src/svn/trunk/gcc/rtlanal.c:3695
0xddf12b simplify_subreg_regno(unsigned int, machine_mode, unsigned int, 
machine_mode)
        /space/rguenther/src/svn/trunk/gcc/rtlanal.c:3808
0xd8bc7a simplifiable_subregs(subreg_shape const&)
        /space/rguenther/src/svn/trunk/gcc/reginfo.c:1234
0xd8be1d record_subregs_of_mode
        /space/rguenther/src/svn/trunk/gcc/reginfo.c:1294
0xd8c246 init_subregs_of_mode()
        /space/rguenther/src/svn/trunk/gcc/reginfo.c:1348
0xc1a55c init_costs
        /space/rguenther/src/svn/trunk/gcc/ira-costs.c:2187

which is

  /* This should always pass, otherwise we don't know how to verify
     the constraint.  These conditions may be relaxed but
     subreg_regno_offset would need to be redesigned.  */
  gcc_assert ((GET_MODE_SIZE (xmode) % GET_MODE_SIZE (ymode)) == 0);

Richard.

Re: [PATCH][GCC 7] Remove broken path in extract_bit_field_1

[Jakub Jelinek]

On Mon, Apr 04, 2016 at 03:27:23PM +0200, Richard Biener wrote:
> It ICEs in
> 
> /space/rguenther/src/svn/trunk/gcc/testsuite/gcc.target/i386/pr37870.c:19:1: 
> internal compiler error: in subreg_get_info, at rtlanal.c:3695
> 0xddee5a subreg_get_info(unsigned int, machine_mode, unsigned int, 
> machine_mode, subreg_info*)
>         /space/rguenther/src/svn/trunk/gcc/rtlanal.c:3695
> 0xddf12b simplify_subreg_regno(unsigned int, machine_mode, unsigned int, 
> machine_mode)
>         /space/rguenther/src/svn/trunk/gcc/rtlanal.c:3808
> 0xd8bc7a simplifiable_subregs(subreg_shape const&)
>         /space/rguenther/src/svn/trunk/gcc/reginfo.c:1234
> 0xd8be1d record_subregs_of_mode
>         /space/rguenther/src/svn/trunk/gcc/reginfo.c:1294
> 0xd8c246 init_subregs_of_mode()
>         /space/rguenther/src/svn/trunk/gcc/reginfo.c:1348
> 0xc1a55c init_costs
>         /space/rguenther/src/svn/trunk/gcc/ira-costs.c:2187
> 
> which is
> 
>   /* This should always pass, otherwise we don't know how to verify
>      the constraint.  These conditions may be relaxed but
>      subreg_regno_offset would need to be redesigned.  */
>   gcc_assert ((GET_MODE_SIZE (xmode) % GET_MODE_SIZE (ymode)) == 0);

So perhaps either validate_subreg should check the same thing, or
the extraction could check this.
Anyway, I don't have anything against removing that hunk from
extract_bit_field_1 for GCC7.

	Jakub

Re: [PATCH][GCC 7] Remove broken path in extract_bit_field_1

[Richard Biener]

On Mon, Apr 4, 2016 at 3:33 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Mon, Apr 04, 2016 at 03:27:23PM +0200, Richard Biener wrote:
>> It ICEs in
>>
>> /space/rguenther/src/svn/trunk/gcc/testsuite/gcc.target/i386/pr37870.c:19:1:
>> internal compiler error: in subreg_get_info, at rtlanal.c:3695
>> 0xddee5a subreg_get_info(unsigned int, machine_mode, unsigned int,
>> machine_mode, subreg_info*)
>>         /space/rguenther/src/svn/trunk/gcc/rtlanal.c:3695
>> 0xddf12b simplify_subreg_regno(unsigned int, machine_mode, unsigned int,
>> machine_mode)
>>         /space/rguenther/src/svn/trunk/gcc/rtlanal.c:3808
>> 0xd8bc7a simplifiable_subregs(subreg_shape const&)
>>         /space/rguenther/src/svn/trunk/gcc/reginfo.c:1234
>> 0xd8be1d record_subregs_of_mode
>>         /space/rguenther/src/svn/trunk/gcc/reginfo.c:1294
>> 0xd8c246 init_subregs_of_mode()
>>         /space/rguenther/src/svn/trunk/gcc/reginfo.c:1348
>> 0xc1a55c init_costs
>>         /space/rguenther/src/svn/trunk/gcc/ira-costs.c:2187
>>
>> which is
>>
>>   /* This should always pass, otherwise we don't know how to verify
>>      the constraint.  These conditions may be relaxed but
>>      subreg_regno_offset would need to be redesigned.  */
>>   gcc_assert ((GET_MODE_SIZE (xmode) % GET_MODE_SIZE (ymode)) == 0);
>
> So perhaps either validate_subreg should check the same thing, or
> the extraction could check this.
> Anyway, I don't have anything against removing that hunk from
> extract_bit_field_1 for GCC7.

Now committed.

Richard.

>         Jakub