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


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.

Thanks,

Martin


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