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...)
Thanks,
Martin
