Re: [PATCH] Support for known unknown alignment

[Richard Guenther <rguenther at suse dot de>]

On Mon, 23 Apr 2012, Martin Jambor wrote:

> Hi,
> 
> On Mon, Apr 23, 2012 at 12:50:51PM +0200, Richard Guenther wrote:
> > On Fri, 20 Apr 2012, Martin Jambor wrote:
> > 
> > > Hi,
> > > 
> > > two days ago I talked to Richi on IRC about the functions to determine
> > > the expected alignment of objects and pointers we have and he
> > > suggested that get_object_alignment_1 and get_pointer_alignment_1
> > > should return whether the alignment is actually known and return the
> > > actual alignment in a reference variable (as they currently return
> > > bitpos).
> > > 
> > > I started with this idea, only changed it a little that if the
> > > alignment is unknown, the functions still set the *alignp parameter to
> > > byte alignment because majority of callers would assume that if
> > > alignment is unknown.
> > > 
> > > Quickly I realized that in order for this idea to work, I'd also need
> > > to add the ability to explicitely distinguish between known and
> > > unknown alignments of SSA name pointers stored in ptr_info_def
> > > structure.  Therefore I made the alignment value zero special, meaning
> > > that we do not know and added a number of functions to get and
> > > manipulate these values while taking care of this special value.
> > > 
> > > IIUC, the value zero did not have any really defined meaning
> > > previously, still ccp_finalize in tree-ssa-ccp.c was happy to set it
> > > to that value at least once during bootstrap.  I have not investigated
> > > why because I did not have time, I simply assumed that value means we
> > > do not know, just like when the assumed alignment is one byte.
> > > 
> > > A very similar patch to this one has passed bootstrap and testsuite on
> > > x86_64-linux, bootstrap and testsuite of this exact patch on the same
> > > platform and many others is currently running.
> > > 
> > > I suppose there will be comments, but eventually I'd like to ask for
> > > permission to commit the patch to trunk.
> > 
> > This looks all good apart from
> > 
> > + void
> > + set_ptr_info_alignment (struct ptr_info_def *pi, unsigned int align,
> > +                           unsigned int misalign)
> > + {
> > +   gcc_checking_assert (align != 0);
> > 
> > Why this assert?  If we assert sth then we should assert that
> > misalign & ~(align - 1) == 0 and that align is a power-of-two or 0.
> 
> Because the patch makes zero value special, meaning "we do not know,"
> and my intention was that this should be announced using
> mark_ptr_info_alignment_unknown, mainly to prevent bugs when somebody
> miscalculated the alignment.  Without the assert, the subsystem would
> silently accept zero as its internal flag which would not ICE or
> miscompile stuff, but perhaps unnecessarily pessimistic alignment
> assumptions would be used.  Moreover, the smallest sensible alignment
> is 1, the misalign component is also byte-based (after all, they
> describe pointers) and zero alignment does not really have any
> meaning, or does it?

Hmm, indeed (given the docs mention ptr & (align - 1)).

> I'll add a FIXME to the place where CCP previously attempted setting
> alignment zero, but I won't have time to investigate that anytime
> soon.

Well, CCP simply tracks known-bits and derives the alignment
value from that.  If tem & -tem computes as zero that means
val->mask.low is all zeros.  Thus the if (align == 1) check
should have been if (align <= 1) from the start.  No fixme necessary
I think.

> Asserting alignment is a power of two and that misalign
> is not bigger than align are good ideas.
> 
> > 
> > +   pi->align = align;
> > +   pi->misalign = misalign;
> > + }
> > + 
> > + /* If pointer decribed by PI has known alignment, increase its known
> > +    misalignment by INCREMENT modulo its current alignment.  */
> > + 
> > + void
> > + increment_ptr_info_misalignment (struct ptr_info_def *pi,
> > +                                 unsigned int increment)
> > + {
> > 
> > Hmm, I'd call it adjust_ptr_info_misalignment instead.
> > 
> 
> Fine with me.
> 
> > Ok with that changes.
> > 
> 
> Unfortunately, the testsuite results from ppc64 look like there are
> some problems.  Hopefully they are not too complex or I'll have to
> postpone this for a few weeks as I have more pressing tasks to do now.

Thanks,
Richard.