This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH, RFC] PR 55415 : Pessimistic misalignment from eipa_sra pass


On Thu, Nov 29, 2012 at 1:33 PM, Martin Jambor <mjambor@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 <mjambor@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 <mjambor@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
>


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]