On Thu, Nov 29, 2012 at 1:33 PM, Martin Jambor <mjam...@suse.cz> wrote:
> 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.
Hm, ok, then this was a confusion on my part.
Richard.
> Thanks,
>
> Martin
>
