Re: [PATCH 3/5] The IPA-SRA itself.

[Richard Guenther <rguenther at suse dot de>]

On Thu, 3 Sep 2009, Martin Jambor wrote:

> Hi,
> 
> On Fri, Aug 07, 2009 at 11:50:02AM +0200, Richard Guenther wrote:
> > On Wed, 5 Aug 2009, Martin Jambor wrote:
> > 
> > > Hi,
> > > 
> > > this patch  contains the implementation  of IPA-SRA which is  built on
> > > top of the new intraprocedural SRA.
> > > 
> > > This is a re-submission in  which I incorporated most of the feedback.
> > > Disqualification of  addressable types and  direct uses are  done when
> > > selecting candidates  in the initial  phase.  The check whether  it is
> > > legal  to move  dereferences to  the caller  has been  re-written from
> > > scratch.  It now does not utilize dominators but rather propagates the
> > > dereference  information across the  control flow  graph in  a fashion
> > > similar  to the simplest  constant-propagation algorithms.   In short,
> > > for each  BB and pointer  candidate I note down  in local_dereferences
> > > whether (and  how much of it)  it was dereferenced  there.  Then, when
> > > actually  checking   the  dereference   info,  I  propagate   this  in
> > > global_dereferences to other basic blocks if all of their predecessors
> > > also   are   marked   as   having  dereferenced   the   parameter   in
> > > global_dereferences.  I propagate this information until it stabilizes
> > > and  then check  it for  BBs which  can abort  the progressing  of the
> > > functions  (returns, calls of  non-pure functions,  potential infinite
> > > loops, external exceptions).
> > 
> > I'm still confused about all this complexity and why you need to consider
> > offsets at all.
> 
> The concern here is that the caller might allocate only a part of the
> whole agggregate if it knows only a part is used.

Uh, ok.  Can you add a testcase covering this case?

> > For simplicity I would simply check dereferencing in 
> > ptr_parm_has_direct_uses where you already walk all direct uses:
> > 
> > > +ptr_parm_has_direct_uses (tree parm)
> > > +{
> > > +  imm_use_iterator ui;
> > > +  gimple stmt;
> > > +  tree name = gimple_default_def (cfun, parm);
> > > +  bool ret = false;
> > > +
> > > +  FOR_EACH_IMM_USE_STMT (stmt, ui, name)
> > > +    {
> > > +      if (gimple_assign_single_p (stmt))
> > > +	{
> > > +	  tree rhs = gimple_assign_rhs1 (stmt);
> > > +	  if (rhs == name)
> > > +	    ret = true;
> > > +	  else if (TREE_CODE (rhs) == ADDR_EXPR)
> > > +	    {
> > > +	      do
> > > +		{
> > > +		  rhs = TREE_OPERAND (rhs, 0);
> > > +		}
> > > +	      while (handled_component_p (rhs));
> > > +	      if (INDIRECT_REF_P (rhs) && TREE_OPERAND (rhs, 0) == name)
> > > +		ret = true;
> > > +	    }
> > 
> >         lhs = gimple_assign_lhs (stmt);
> >         if (TREE_CODE (lhs) != SSA_NAME)
> >           {
> >             while (handled_component_p (lhs))
> >              lhs = TREE_OPERAND (lhs, 0);
> >             if (INDIRECT_REF_P (lhs) && TREE_OPERAND (lhs, 0) == name
> >                 && (gimple_bb (stmt) == single_succ 
> > (ENTRY_BLOCK_PTR)
> >                     || dominated_by_p (CDI_POST_DOMINATORS,
> > 				   single_succ (ENTRY_BLOCK_PTR),
> >                                    gimple_bb (stmt))))
> >               {
> >                 note parm can be dereferenced in the caller
> >               }
> >           }
> > 
> >         same for the rhs (and maybe call lhs and call args, but that
> >         probably doesn't make too much of a difference).
> > 
> 
> As far as I can remember, me and Honza (actually, the whole algorithm
> was Honza's idea, and most probably a good one) have decided not to
> rely on dominance but rather do it in this way because of cases like
> this:
> 
>   if (condition)
>     {
>       a = param->field + 1;
>       /* maybe do a lot of othe stuff */
>     }
>   else
>     {
>       a = param->field + 5;
>       /* maybe do a lot of othe stuff */
>     }
> 
> Neither of the loads (dereferences) post-dominates exit but it is
> still safe to proceed with dereferencing in the caller.  In the test
> cases I have examined the loads remained in the branches and were not
> moved before the condition.  Moreover, the scenario seems very frequent.

Hmm, ok.

I don't quite follow analyze_caller_derference_legality though
(or rather local vs global dereferences).  Local dereferences are
built up during access building and in a_c_d_l you propagate them
into the global ones.  Why do you update local_dereferences i
during this propagation?  In fact - why have two arrays anyway and
not just propagate in-place?  Also in the caller of a_c_d_l you
check for each BB and for each param whether the group is not
necessarily dereferenced - but you just propagated the offsets, so
checking the entry basic block should be enough?

That I don't get it means it lacks documentation - what is the
converged state after propagation supposed to look like?  Thus, what
can you actually do with it?

Thanks,
Richard.