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.