PR target/30383 (ICE in i386 stringops rewrite)

PR target/30383 (ICE in i386 stringops rewrite)

[Jan Hubicka]

Hi,
the ICE is ccaused by middle end passing alignment of 0 to i386 backend for
string operations.  Since the representation of alignment here is in bytes,
value of 0 is not meaningful.  

This is caused by fact that LABEL_DECLs have alignment set to 1 (in bits) that
is almost certainly wrong.  The catch is that I am not quite sure where the
alignment shall be done - all labels seems to be build by build_decl and they
are never layouted like variables.

The following patch set initialization to make_node_stat that is already
special casing functions, so it might be good place for it.  I also have tested
alternative patch that simply makes i386 backend to tolerate this, but it seems
like middle-end omision and I don't think it is possible to construct particularly
evil different testcase where middle-end would be right to claim sub unit pointer
just because the argument is pointer.

Bootstrapped/regtested i686-linux, thanks to pinskia for testcase, OK?
 void jumpfunc(int copy, void *p)
{
__builtin_memcpy (p, &&dummy, 128);
 dummy: ;
}
	PR target/30383
	* tree.c (make_node_stat): Initialize alignments of labels.
Index: tree.c
===================================================================
*** tree.c	(revision 120528)
--- tree.c	(working copy)
*************** make_node_stat (enum tree_code code MEM_
*** 589,595 ****
  	DECL_IN_SYSTEM_HEADER (t) = in_system_header;
        if (CODE_CONTAINS_STRUCT (code, TS_DECL_COMMON))
  	{
! 	  if (code != FUNCTION_DECL)
  	    DECL_ALIGN (t) = 1;
  	  DECL_USER_ALIGN (t) = 0;	  
  	  /* We have not yet computed the alias set for this declaration.  */
--- 589,597 ----
  	DECL_IN_SYSTEM_HEADER (t) = in_system_header;
        if (CODE_CONTAINS_STRUCT (code, TS_DECL_COMMON))
  	{
! 	  if (code == LABEL_DECL)
! 	    DECL_ALIGN (t) = BITS_PER_UNIT;
! 	  else if (code != FUNCTION_DECL)
  	    DECL_ALIGN (t) = 1;
  	  DECL_USER_ALIGN (t) = 0;	  
  	  /* We have not yet computed the alias set for this declaration.  */

Re: PR target/30383 (ICE in i386 stringops rewrite)

[Andrew Pinski]

On Mon, 2007-01-08 at 14:10 +0100, Jan Hubicka wrote:
> !         if (code == LABEL_DECL)
> !           DECL_ALIGN (t) = BITS_PER_UNIT; 

This is incorrect as you using bits here instead of bytes (units).
You will get an alignment of "8" which is incorrect for some cases.
DECL_ALIGN of 1 should be right but the question now is, where we change
that into 0?

Thanks,
Andrew Pinski

Re: PR target/30383 (ICE in i386 stringops rewrite)

[Jan Hubicka]

> On Mon, 2007-01-08 at 14:10 +0100, Jan Hubicka wrote:
> > !         if (code == LABEL_DECL)
> > !           DECL_ALIGN (t) = BITS_PER_UNIT; 
> 
> This is incorrect as you using bits here instead of bytes (units).
> You will get an alignment of "8" which is incorrect for some cases.
> DECL_ALIGN of 1 should be right but the question now is, where we change
> that into 0?

The emit_block_move expander takes alignment in bits, while passes it in
bytes to RTL expanders, so it is divided by BITS_PER_UNIT and rounded
down.
I think we should also gcc_assert there that alignment % BITS_PER_UNIT
== 0, I will add this sanity check and regtest.  We can't move/clear at
sub byte basis.

Honza
> 
> Thanks,
> Andrew Pinski

Re: PR target/30383 (ICE in i386 stringops rewrite)

[Eric Christopher]

>
> ! 	  if (code == LABEL_DECL)
> ! 	    DECL_ALIGN (t) = BITS_PER_UNIT;
> ! 	  else if (code != FUNCTION_DECL)
>

I think you want bytes not bits here.

-eric

Re: PR target/30383 (ICE in i386 stringops rewrite)

[Jan Hubicka]

> >
> >! 	  if (code == LABEL_DECL)
> >! 	    DECL_ALIGN (t) = BITS_PER_UNIT;
> >! 	  else if (code != FUNCTION_DECL)
> >
> 
> I think you want bytes not bits here.

DECL_ALIGN is bit based:
/* Holds the alignment required for the datum, in bits.  */
#define DECL_ALIGN(NODE) (DECL_COMMON_CHECK (NODE)->decl_common.u1.a.align)

Honza
> 
> -eric

Re: PR target/30383 (ICE in i386 stringops rewrite)

[Eric Christopher]

On Jan 8, 2007, at 6:41 PM, Jan Hubicka wrote:

>>>
>>> ! 	  if (code == LABEL_DECL)
>>> ! 	    DECL_ALIGN (t) = BITS_PER_UNIT;
>>> ! 	  else if (code != FUNCTION_DECL)
>>>
>>
>> I think you want bytes not bits here.
>
> DECL_ALIGN is bit based:
> /* Holds the alignment required for the datum, in bits.  */

Hunh. Brain fart. The real question probably should have been "Are you  
sure bits_per_unit is right?"

thanks.

-eric

Re: PR target/30383 (ICE in i386 stringops rewrite)

[Jan Hubicka]

> 
> On Jan 8, 2007, at 6:41 PM, Jan Hubicka wrote:
> 
> >>>
> >>>! 	  if (code == LABEL_DECL)
> >>>! 	    DECL_ALIGN (t) = BITS_PER_UNIT;
> >>>! 	  else if (code != FUNCTION_DECL)
> >>>
> >>
> >>I think you want bytes not bits here.
> >
> >DECL_ALIGN is bit based:
> >/* Holds the alignment required for the datum, in bits.  */
> 
> Hunh. Brain fart. The real question probably should have been "Are you  
> sure bits_per_unit is right?"

I would say it is (at least in conservative way) - anything smaller than
BITS_PER_UNIT is wrong.  While many architectures mandate greater
alignment I don't see this being interfaced to middle end and I don't
think it really matters.  

Honza
> 
> thanks.
> 
> -eric

Re: PR target/30383 (ICE in i386 stringops rewrite)

[Eric Christopher]

On Jan 9, 2007, at 4:21 AM, Jan Hubicka wrote:

>>
>> On Jan 8, 2007, at 6:41 PM, Jan Hubicka wrote:
>>
>>>>>
>>>>> ! 	  if (code == LABEL_DECL)
>>>>> ! 	    DECL_ALIGN (t) = BITS_PER_UNIT;
>>>>> ! 	  else if (code != FUNCTION_DECL)
>>>>>
>>>>
>>>> I think you want bytes not bits here.
>>>
>>> DECL_ALIGN is bit based:
>>> /* Holds the alignment required for the datum, in bits.  */
>>
>> Hunh. Brain fart. The real question probably should have been "Are  
>> you
>> sure bits_per_unit is right?"
>
> I would say it is (at least in conservative way) - anything smaller  
> than
> BITS_PER_UNIT is wrong.  While many architectures mandate greater
> alignment I don't see this being interfaced to middle end and I don't
> think it really matters.

OK. Sounds good.

-eric

Re: PR target/30383 (ICE in i386 stringops rewrite)

[Roger Sayle]

On Mon, 8 Jan 2007, Jan Hubicka wrote:
>
> 	PR target/30383
> 	* tree.c (make_node_stat): Initialize alignments of labels.

This is OK for mainline, provided that you also add a suitable version
of Andrew's testcase to the testsuite.  I'm also a fan of RTH's philosphy
of fixing "interface" bugs in both places, so I'd be interested in seeing
what you i386 backend fix looks like.

I'm a little surprised that we don't have a target macro to describe
label alignment.  Whilst it's true that on x86/x86_64, instructions
are only byte aligned, on many RISC-like targets they have 32-bit
alignment, on IA-64 I believe they are 64-bit aligned and on AVR they
are 16-bit aligned.  Seems like this could be usefully used by the
optimizers and by memset/memcpy expansion on other targets?

Thanks for fixing this.

Roger
--