Re: [PATCH] Support for known unknown alignment

[Martin Jambor <mjambor at suse dot cz>]

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?

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.
 
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,

Martin