Patchwork Disable aggregate jump functions for bit-field stores

login
register
mail settings
Submitter Martin Jambor
Date Nov. 4, 2012, 10:39 p.m.
Message ID <20121104223946.GC5617@virgil.arch.suse.de>
Download mbox | patch
Permalink /patch/197112/
State New
Headers show

Comments

Martin Jambor - Nov. 4, 2012, 10:39 p.m.
Hi,

the patch below disables generation of aggregate jump functions from
bit-field stores because currently we depend on type size of the value
to determine the size of the stored value and that does not work with
bit-fields, making it impossible for IPA-CP to organize them in their
lattices.

If we ever decide aggregate jump functions for bit-fields are worth
the hassle, we might remove this limitation by storing and streaming
the size of the memory reference alongside the offset in the jump
functions (and IPA-CP lattices).

Bootstrapped and tested on x86_64-linux, needed for the aggregate
IPA-CP.  OK for trunk?

Thanks,

Martin


2012-11-02  Martin Jambor  <mjambor@suse.cz>

	* ipa-prop.c (determine_known_aggregate_parts): Do not create
	aggregate jump functions for bit-fields.
Jan Hubicka - Nov. 5, 2012, 4:20 p.m.
> Hi,
> 
> the patch below disables generation of aggregate jump functions from
> bit-field stores because currently we depend on type size of the value
> to determine the size of the stored value and that does not work with
> bit-fields, making it impossible for IPA-CP to organize them in their
> lattices.
> 
> If we ever decide aggregate jump functions for bit-fields are worth
> the hassle, we might remove this limitation by storing and streaming
> the size of the memory reference alongside the offset in the jump
> functions (and IPA-CP lattices).
> 
> Bootstrapped and tested on x86_64-linux, needed for the aggregate
> IPA-CP.  OK for trunk?
> 
> Thanks,
> 
> Martin
> 
> 
> 2012-11-02  Martin Jambor  <mjambor@suse.cz>
> 
> 	* ipa-prop.c (determine_known_aggregate_parts): Do not create
> 	aggregate jump functions for bit-fields.
> 
> Index: src/gcc/ipa-prop.c
> ===================================================================
> --- src.orig/gcc/ipa-prop.c
> +++ src/gcc/ipa-prop.c
> @@ -1295,7 +1295,10 @@ determine_known_aggregate_parts (gimple
>  
>        lhs = gimple_assign_lhs (stmt);
>        rhs = gimple_assign_rhs1 (stmt);
> -      if (!is_gimple_reg_type (rhs))
> +      if (!is_gimple_reg_type (rhs)
> +          || TREE_CODE (lhs) == BIT_FIELD_REF
> +	  || (TREE_CODE (lhs) == COMPONENT_REF
> +	      && DECL_BIT_FIELD (TREE_OPERAND (lhs, 1))))

I am not sure I understand motivation of this patch properly.  First I think
BIT_FIELD_REF can be hidden inside chain of other REFs so you should probably
look into all handled components.  What exactly goes wrong when your type size
is bigger than the bitfield?

Honza
Martin Jambor - Nov. 5, 2012, 5:40 p.m.
On Mon, Nov 05, 2012 at 05:20:46PM +0100, Jan Hubicka wrote:
> > Hi,
> > 
> > the patch below disables generation of aggregate jump functions from
> > bit-field stores because currently we depend on type size of the value
> > to determine the size of the stored value and that does not work with
> > bit-fields, making it impossible for IPA-CP to organize them in their
> > lattices.
> > 
> > If we ever decide aggregate jump functions for bit-fields are worth
> > the hassle, we might remove this limitation by storing and streaming
> > the size of the memory reference alongside the offset in the jump
> > functions (and IPA-CP lattices).
> > 
> > Bootstrapped and tested on x86_64-linux, needed for the aggregate
> > IPA-CP.  OK for trunk?
> > 
> > Thanks,
> > 
> > Martin
> > 
> > 
> > 2012-11-02  Martin Jambor  <mjambor@suse.cz>
> > 
> > 	* ipa-prop.c (determine_known_aggregate_parts): Do not create
> > 	aggregate jump functions for bit-fields.
> > 
> > Index: src/gcc/ipa-prop.c
> > ===================================================================
> > --- src.orig/gcc/ipa-prop.c
> > +++ src/gcc/ipa-prop.c
> > @@ -1295,7 +1295,10 @@ determine_known_aggregate_parts (gimple
> >  
> >        lhs = gimple_assign_lhs (stmt);
> >        rhs = gimple_assign_rhs1 (stmt);
> > -      if (!is_gimple_reg_type (rhs))
> > +      if (!is_gimple_reg_type (rhs)
> > +          || TREE_CODE (lhs) == BIT_FIELD_REF
> > +	  || (TREE_CODE (lhs) == COMPONENT_REF
> > +	      && DECL_BIT_FIELD (TREE_OPERAND (lhs, 1))))
> 
> I am not sure I understand motivation of this patch properly.  First I think
> BIT_FIELD_REF can be hidden inside chain of other REFs so you should probably
> look into all handled components.  What exactly goes wrong when your type size
> is bigger than the bitfield?
> 

I hit an assert in the patch I am about to post (I am writing the
changelog right now) I hit an assert checking that stuff does not
overlap in IPA-CP aggregate lattices.  My initial reaction was that I
did not want to deal with bit-fields, unless really necessary ;-)

However, that assert can be easily changed to bottomizing the lattice,
I will re-test with that and if there are no problems, this patch can
be omitted.

Thanks,

Martin

Patch

Index: src/gcc/ipa-prop.c
===================================================================
--- src.orig/gcc/ipa-prop.c
+++ src/gcc/ipa-prop.c
@@ -1295,7 +1295,10 @@  determine_known_aggregate_parts (gimple
 
       lhs = gimple_assign_lhs (stmt);
       rhs = gimple_assign_rhs1 (stmt);
-      if (!is_gimple_reg_type (rhs))
+      if (!is_gimple_reg_type (rhs)
+          || TREE_CODE (lhs) == BIT_FIELD_REF
+	  || (TREE_CODE (lhs) == COMPONENT_REF
+	      && DECL_BIT_FIELD (TREE_OPERAND (lhs, 1))))
 	break;
 
       lhs_base = get_ref_base_and_extent (lhs, &lhs_offset, &lhs_size,