Hi,
On Thu, Nov 29, 2012 at 12:54:20PM +0100, Richard Biener wrote:
> On Thu, Nov 29, 2012 at 11:06 AM, Martin Jambor <mjam...@suse.cz> wrote:
> > Hi,
> >
> > thanks for the review. When writing a reply I realized I indeed made
> > a mistake or two in the part concerning prev_base and the code was not
> > what it intended to be. I'll re-write it today.
> >
> > Nevertheless, I also have a question regarding a different place of
> > the patch:
> >
> > On Wed, Nov 28, 2012 at 02:11:51PM +0100, Richard Biener wrote:
> >> On Tue, Nov 27, 2012 at 9:37 PM, Martin Jambor <mjam...@suse.cz> wrote:
> >> > *** /tmp/Hp0Wyc_tree-sra.c Tue Nov 27 21:34:54 2012
> >> > --- gcc/tree-sra.c Tue Nov 27 21:28:53 2012
> >> > *************** unmodified_by_ref_scalar_representative
> >> > *** 3891,3902 ****
> >> > return repr;
> >> > }
> >> >
> >> > ! /* Return true iff this access precludes IPA-SRA of the parameter it is
> >> > ! associated with. */
> >> >
> >> > static bool
> >> > ! access_precludes_ipa_sra_p (struct access *access)
> >> > {
> >> > /* Avoid issues such as the second simple testcase in PR 42025. The
> >> > problem
> >> > is incompatible assign in a call statement (and possibly even in
> >> > asm
> >> > statements). This can be relaxed by using a new temporary but
> >> > only for
> >> > --- 3891,3903 ----
> >> > return repr;
> >> > }
> >> >
> >> > ! /* Return true iff this ACCESS precludes IPA-SRA of the parameter it is
> >> > ! associated with. REQ_ALIGN is the minimum required alignment. */
> >> >
> >> > static bool
> >> > ! access_precludes_ipa_sra_p (struct access *access, unsigned int
> >> > req_align)
> >> > {
> >> > + unsigned int exp_align;
> >> > /* Avoid issues such as the second simple testcase in PR 42025. The
> >> > problem
> >> > is incompatible assign in a call statement (and possibly even in
> >> > asm
> >> > statements). This can be relaxed by using a new temporary but
> >> > only for
> >> > *************** access_precludes_ipa_sra_p (struct acces
> >> > *** 3908,3913 ****
> >> > --- 3909,3918 ----
> >> > || gimple_code (access->stmt) == GIMPLE_ASM))
> >> > return true;
> >> >
> >> > + exp_align = get_object_alignment (access->expr);
> >>
> >> Is access->expr from the callee or the caller?
> >
> > The callee.
> >
> >>
> >> > + if (exp_align < req_align)
> >> > + return true;
> >> > +
> >> > return false;
> >> > }
> >> >
> >> > *************** splice_param_accesses (tree parm, bool *
> >> > *** 3943,3949 ****
> >> > tree a1_alias_type;
> >> > access = (*access_vec)[i];
> >> > modification = access->write;
> >> > ! if (access_precludes_ipa_sra_p (access))
> >> > return NULL;
> >> > a1_alias_type = reference_alias_ptr_type (access->expr);
> >> >
> >> > --- 3948,3954 ----
> >> > tree a1_alias_type;
> >> > access = (*access_vec)[i];
> >> > modification = access->write;
> >> > ! if (access_precludes_ipa_sra_p (access, TYPE_ALIGN
> >> > (access->type)))
> >>
> >> access->type is not the type of the base object, right?
> >
> > No, it is the actual type of the scalar memory access.
> >
> >> Which means it is not correct here - the alignment of the access is that
> >> what
> >> get_object_alignment returns, which looks at the base as to whether to
> >> lower the alignment compared to TYPE_ALIGN of the access type itself.
> >
> > Oh, so I have to do something like (or even reuse) may_be_unaligned_p
> > from tree-ssa-loop-ivopts.c? (If so, I'm wondering whether a few other
> > uses of get_object_alignment get this right...)
>
> Well, I think you should use get_object_alignment here, too, and punt if
> that is less than TYPE_ALIGN (access->type) if you will end up using that.
>
> get_object_alignment (ref) is the authorative answer to alignment questions.
> Whether TYPE_ALIGN (TREE_TYPE (ref)) can be conservatively trusted
> depends on whether get_object_alignment (ref) returns the same or bigger
> alignment. But if the type is all you can propagate in this case (and not
> an explicit alignment value) you have to punt if the information from the
> type isn't conservative.
>
This must be some sort of misunderstanding, that is exactly what the
code is doing. access_precludes_ipa_sra_p called get_object_alignment
on the reference and then returned false (i.e. prevented the
transformation) if the result was smaller than the TYPE_ALIGN of the
type that was going to be propagated to the callee.
Thanks,
Martin
