Patchwork Continue strict-volatile-bitfields fixes

login
register
mail settings
Submitter Thomas Schwinge
Date May 9, 2012, 2:01 a.m.
Message ID <874nrqaz8s.fsf@schwinge.name>
Download mbox | patch
Permalink /patch/157846/
State Accepted, archived
Headers show

Comments

Thomas Schwinge - May 9, 2012, 2:01 a.m.
Hi!

On Fri, 27 Apr 2012 10:29:06 +0200, Jakub Jelinek <jakub@redhat.com> wrote:
> On Fri, Apr 27, 2012 at 12:42:41PM +0800, Thomas Schwinge wrote:
> > > > GET_MODE_BITSIZE (lmode)« (8 bits).  (With the current sources, lmode is
> > > > VOIDmode.)
> > > >
> > > > Is emmitting »BIT_FIELD_REF <*common, 32, 0> & 255« wrong in this case,
> > > > or should a later optimization pass be able to figure out that
> > > > »BIT_FIELD_REF <*common, 32, 0> & 255« is in fact the same as
> > > > common->code, and then be able to conflate these?  Any suggestions
> > > > where/how to tackle this?
> > > 
> > > The BIT_FIELD_REF is somewhat of a red-herring.  It is created by fold-const.c
> > > in optimize_bit_field_compare, code that I think should be removed completely.
> > > Or it needs to be made aware of strict-volatile bitfield and C++ memory model
> > > details.
> 
> I'd actually very much prefer the latter, just disable
> optimize_bit_field_compare for strict-volatile bitfield mode and when
> avoiding load data races in C++ memory model (that isn't going to be
> default, right?).  This optimization is useful, and it is solely about
> loads, so even C++ memory model usually shouldn't care.

I can't comment on the C++ memory model bits, but I have now tested the
following patch (fixes the issue for SH, no regressions for ARM, x86):

gcc/
	* fold-const.c (optimize_bit_field_compare): Abort early in the strict
	volatile bitfields case.



Grüße,
 Thomas
Thomas Schwinge - May 16, 2012, 5:14 p.m.
Hi!

Ping.

On Wed, 09 May 2012 10:01:55 +0800, I wrote:
> On Fri, 27 Apr 2012 10:29:06 +0200, Jakub Jelinek <jakub@redhat.com> wrote:
> > On Fri, Apr 27, 2012 at 12:42:41PM +0800, Thomas Schwinge wrote:
> > > > > GET_MODE_BITSIZE (lmode)« (8 bits).  (With the current sources, lmode is
> > > > > VOIDmode.)
> > > > >
> > > > > Is emmitting »BIT_FIELD_REF <*common, 32, 0> & 255« wrong in this case,
> > > > > or should a later optimization pass be able to figure out that
> > > > > »BIT_FIELD_REF <*common, 32, 0> & 255« is in fact the same as
> > > > > common->code, and then be able to conflate these?  Any suggestions
> > > > > where/how to tackle this?
> > > > 
> > > > The BIT_FIELD_REF is somewhat of a red-herring.  It is created by fold-const.c
> > > > in optimize_bit_field_compare, code that I think should be removed completely.
> > > > Or it needs to be made aware of strict-volatile bitfield and C++ memory model
> > > > details.
> > 
> > I'd actually very much prefer the latter, just disable
> > optimize_bit_field_compare for strict-volatile bitfield mode and when
> > avoiding load data races in C++ memory model (that isn't going to be
> > default, right?).  This optimization is useful, and it is solely about
> > loads, so even C++ memory model usually shouldn't care.
> 
> I can't comment on the C++ memory model bits, but I have now tested the
> following patch (fixes the issue for SH, no regressions for ARM, x86):
> 
> gcc/
> 	* fold-const.c (optimize_bit_field_compare): Abort early in the strict
> 	volatile bitfields case.
> 
> Index: fold-const.c
> ===================================================================
> --- fold-const.c	(revision 186856)
> +++ fold-const.c	(working copy)
> @@ -3342,6 +3342,11 @@ optimize_bit_field_compare (location_t loc, enum t
>    tree mask;
>    tree offset;
>  
> +  /* In the strict volatile bitfields case, doing code changes here may prevent
> +     other optimizations, in particular in a SLOW_BYTE_ACCESS setting.  */
> +  if (flag_strict_volatile_bitfields > 0)
> +    return 0;
> +
>    /* Get all the information about the extractions being done.  If the bit size
>       if the same as the size of the underlying object, we aren't doing an
>       extraction at all and so can do nothing.  We also don't want to


Grüße,
 Thomas
Thomas Schwinge - May 24, 2012, 12:29 p.m.
Hi!

Ping.

On Wed, 16 May 2012 19:14:45 +0200, I wrote:
> Ping.
> 
> On Wed, 09 May 2012 10:01:55 +0800, I wrote:
> > On Fri, 27 Apr 2012 10:29:06 +0200, Jakub Jelinek <jakub@redhat.com> wrote:
> > > On Fri, Apr 27, 2012 at 12:42:41PM +0800, Thomas Schwinge wrote:
> > > > > > GET_MODE_BITSIZE (lmode)« (8 bits).  (With the current sources, lmode is
> > > > > > VOIDmode.)
> > > > > >
> > > > > > Is emmitting »BIT_FIELD_REF <*common, 32, 0> & 255« wrong in this case,
> > > > > > or should a later optimization pass be able to figure out that
> > > > > > »BIT_FIELD_REF <*common, 32, 0> & 255« is in fact the same as
> > > > > > common->code, and then be able to conflate these?  Any suggestions
> > > > > > where/how to tackle this?
> > > > > 
> > > > > The BIT_FIELD_REF is somewhat of a red-herring.  It is created by fold-const.c
> > > > > in optimize_bit_field_compare, code that I think should be removed completely.
> > > > > Or it needs to be made aware of strict-volatile bitfield and C++ memory model
> > > > > details.
> > > 
> > > I'd actually very much prefer the latter, just disable
> > > optimize_bit_field_compare for strict-volatile bitfield mode and when
> > > avoiding load data races in C++ memory model (that isn't going to be
> > > default, right?).  This optimization is useful, and it is solely about
> > > loads, so even C++ memory model usually shouldn't care.
> > 
> > I can't comment on the C++ memory model bits, but I have now tested the
> > following patch (fixes the issue for SH, no regressions for ARM, x86):
> > 
> > gcc/
> > 	* fold-const.c (optimize_bit_field_compare): Abort early in the strict
> > 	volatile bitfields case.
> > 
> > Index: fold-const.c
> > ===================================================================
> > --- fold-const.c	(revision 186856)
> > +++ fold-const.c	(working copy)
> > @@ -3342,6 +3342,11 @@ optimize_bit_field_compare (location_t loc, enum t
> >    tree mask;
> >    tree offset;
> >  
> > +  /* In the strict volatile bitfields case, doing code changes here may prevent
> > +     other optimizations, in particular in a SLOW_BYTE_ACCESS setting.  */
> > +  if (flag_strict_volatile_bitfields > 0)
> > +    return 0;
> > +
> >    /* Get all the information about the extractions being done.  If the bit size
> >       if the same as the size of the underlying object, we aren't doing an
> >       extraction at all and so can do nothing.  We also don't want to


Grüße,
 Thomas
Jakub Jelinek - May 24, 2012, 12:36 p.m.
On Thu, May 24, 2012 at 02:29:18PM +0200, Thomas Schwinge wrote:
> Hi!
> 
> Ping.

Ok.

> > > 	* fold-const.c (optimize_bit_field_compare): Abort early in the strict
> > > 	volatile bitfields case.
> > > 
> > > Index: fold-const.c
> > > ===================================================================
> > > --- fold-const.c	(revision 186856)
> > > +++ fold-const.c	(working copy)
> > > @@ -3342,6 +3342,11 @@ optimize_bit_field_compare (location_t loc, enum t
> > >    tree mask;
> > >    tree offset;
> > >  
> > > +  /* In the strict volatile bitfields case, doing code changes here may prevent
> > > +     other optimizations, in particular in a SLOW_BYTE_ACCESS setting.  */
> > > +  if (flag_strict_volatile_bitfields > 0)
> > > +    return 0;
> > > +
> > >    /* Get all the information about the extractions being done.  If the bit size
> > >       if the same as the size of the underlying object, we aren't doing an
> > >       extraction at all and so can do nothing.  We also don't want to

	Jakub

Patch

Index: fold-const.c
===================================================================
--- fold-const.c	(revision 186856)
+++ fold-const.c	(working copy)
@@ -3342,6 +3342,11 @@  optimize_bit_field_compare (location_t loc, enum t
   tree mask;
   tree offset;
 
+  /* In the strict volatile bitfields case, doing code changes here may prevent
+     other optimizations, in particular in a SLOW_BYTE_ACCESS setting.  */
+  if (flag_strict_volatile_bitfields > 0)
+    return 0;
+
   /* Get all the information about the extractions being done.  If the bit size
      if the same as the size of the underlying object, we aren't doing an
      extraction at all and so can do nothing.  We also don't want to