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