Re: [RFC] New SRA and early interprocedural SRA

[Martin Jambor <mjambor at suse dot cz>]

Hi,

thanks for your comments, I have addressed some of them, I am not sure
what do to with another few.

On Fri, Mar 06, 2009 at 12:29:07PM +0100, Jan Hubicka wrote:
> Hi,
> patch is OK for pretty-IPA with following changes (and changelog :)

What about  the current tree-sra.c  and the nastiness of  the passes.c
hunk?   Should I  replace the  previous intrsprocedural  SRA outright?
That would make it difficult for us to assess run time benefits of any
subsequent  patches.  Should  I keep  it as  it is?   Or  do something
different about it?

> > +#include "params.h"
> > +#include "target.h"
> > +#include "ipa-utils.h"
> > +
> > +enum sra_mode {SRA_MODE_EARLY_IPA, SRA_MODE_EARLY_INTRA,
> > +	       SRA_MODE_INTRA} mode;
> 
> make it static, name it in a bit more descriptive way, "mode" rings the
> bell to those still remembering RTL :)
> Also comment and indent it same way we indent enums elsewhere.

OK

> > +
> > +/* Representative of no accesses at all. */
> > +static struct access no_accesses_representant;
> > +
> > +/* Predicate to test the special value.  */
> > +
> > +static inline bool
> > +no_accesses_p (struct access *access)
> > +{
> > +  return access == &no_accesses_representant;
> > +}
> Move this down to place where other functions are... it seems a bit lost
> here in between delcarations :)

OK

> > +
> > +/* Structure for the final plan what to do in IPA modes.  */
> > +struct new_parm_note
> > +{
> > +  tree base;
> > +  tree type;
> > +  tree reduction;
> > +  tree new_ssa_base;
> > +  HOST_WIDE_INT offset;
> > +
> > +  int stmt_no;
> > +
> > +  unsigned copy_param : 1; 	/* Untouched copied parameter. */
> > +  unsigned remove_param : 1;	/* Remove an unused parameter. */
> > +  unsigned last_reduction : 1;	/* Last reduction component of an original
> > +				   parameter.  */
> > +  unsigned by_ref : 1;		/* Create a parameter passed by reference? */
> > +};
> 
> As discussed earlier, for IPA clonning we want to move this to some
> separate place, so IPA-CP and others can use it too.  
> Perhaps to ipa.c or new ipa-param.c depending on how long the
> infrastructure is?

Even though  its name is not very  descriptive, ipa-prop.[ch] contains
stuff  related  to parameters  which  are  shared  among a  number  of
different  transformations  (ipa-cp   and  indirect  inlining  at  the
moment).  I guess I'll move it there, perhaps we should rename it.

Anyway, I am  about to start working on this.  I'll  write down what I
think should be done and send to you first.

> > +
> > +/* We employ very simplistic control flow sensitivity in our early ipa SRA
> > +   analysis.  SAFE_BB is very first basic block of function if there is no loop
> > +   edge reaching it.  STMT_NO is number of statement in this BB or -1.  This
> > +   way we can scan if memory write must happen after last read of argument.  */
> > +static basic_block safe_bb;
> > +static int stmt_no;
> 
> I think it might be very usefull to extend this to work across
> conditionals.  There is already some function that escapes my name to
> initialize stmt uids in dominator order.  You can use those UIDs and
> mark all BBs safe that can be reached from entry with no backward edge
> in DFS ordering.
> This can be done incrementally :)

Right.  Moreover, so far I have not been able to find such a function
by grepping.

> 
> > +
> > +/* Mark all virtual operands of a statement STMT for renaming.  */
> > +
> > +static void
> > +update_all_vops (gimple stmt)
> 
> isn't mark_symbols_for_renaming (stmt) doing same job?

Apparently not. Current tree-sra.c has to do this too.

> 
> > +	      /* FIXME: Try to use some alias information so that we can be
> > +		 less conservative.  */
> 
> Please check it, but I think we can consider safe C++ hidden references
> marked via DECL_BY_REFERENCE.  I think those are safe to keep since
> compiler is required to make a copy of object (because there is copy
> constructor).

I have added this to my todo list.

> > +
> > +/* Check whether any representative (in a linked list pointed to by
> > +   REPRESENTATIVES) is potentially modified by a call statement and mark it so
> > +   if it is.  Note: LHS of the statement is not checked because that is
> > +   recorded automatically then the corresponding access is created.  */
> > +
> > +static inline void
> > +check_call (VEC (access_p, heap) *representatives, gimple call)
> > +{
> > +  int flags = gimple_call_flags (call);
> > +  tree callee_t = gimple_call_fndecl (call);
> > +
> > +  if (flags & (ECF_CONST | ECF_PURE))
> > +    return;
> > +  /* Recursive calls are safe.  */
> 
> I am not quite certain about this anymore, especially with the flow
> sensitivity that essentially means adding loopback edge.
> 
> Consider
> 
> a(*obj1, *obj2)
> {
>   a(obj2,obj1);

Such  direct  use  of the  pointers  would  not  allow IPA-SRA  to  be
performed on  them and so this  function would not be  modified in any
way.  Therefore I don't understand your concern.

>   *obj2 = sdfsdafs;
> }
> 
> Such function modify both obj1 and obj2 but with skipping recursive
> calls one fails to handle obj2?
> > +
> > +/* Return true iff TYPE is one of the types we consider aggregate for SRA
> > +   tranformations.  */
> > +
> > +static bool
> > +sra_aggregate_type_p (tree type)
> 
> probably mark this as inline.  We are still compiled with -O2 that means
> that such functio might be considered too big :)

OK

> > +
> > +/* Return true iff the type contains a field or element type which dpoes not
> > +   allow scalarization.  */
> > +
> > +static bool
> > +type_internals_preclude_sra_p (tree type)
> > +{
> > +  tree fld;
> > +  tree et;
> > +
> > +  switch (TREE_CODE (type))
> > +    {
> > +    case RECORD_TYPE:
> > +    case UNION_TYPE:
> > +    case QUAL_UNION_TYPE:
> Probably it would be more readable with AGGREGATE_TYPE_P predicate
> rather than listing them all.

Well, ARRAY_TYPE is not listed here and is handled differently below
it whereas AGGREGATE_TYPE_P does return true for it too.

> > +
> > +      if (POINTER_TYPE_P (type))
> > +	{
> > +	  type = TREE_TYPE (type);
> > +
> Probably comment here why those fails and why for example UNION_TYPE
> does not work?

Hm, fair enough, I  have forgot to add unions here.  I  will give it a
go.   I am  not sure  what else  there is  to comment,  why  I exclude
pointers to functions?

> > +	  if ((!is_gimple_reg_type (type)
> > +	       && TREE_CODE (type) != RECORD_TYPE
> > +	       && TREE_CODE (type) != ARRAY_TYPE)
> > +	      || TREE_CODE (type) == FUNCTION_TYPE)
> > +	    continue;
> 
> > +/* Helper of QSORT function. There are pointers to accesses in the array.  An
> > +   access is considered smaller than another if it has smaller offset or if the
> > +   offsets are the same but is size is bigger. */
> > +
> > +static int
> > +compare_access_positions (const void *a, const void *b)
> > +{
> > +  const access_p *fp1 = (const access_p *) a;
> > +  const access_p *fp2 = (const access_p *) b;
> > +  const access_p f1 = *fp1;
> > +  const access_p f2 = *fp2;
> > +
> > +  if ((int) f1->offset != (int) f2->offset)
> > +    return (int) f1->offset - (int) f2->offset;
> 
> This probably should all do arithmetic in HOST_WIDE_INT arithmetic
> returning just -1,0,1 to care overflows?

Right.  What about thew following?

  if (f1->offset != f2->offset)
    return (int) (f1->offset - f2->offset);
  /* We want the bigger accesses first, thus the opposite order in the next
     line: */
  return (int) (f2->size - f1->size);


> > +
> > +/* Return true if TYPE is a scalar type.  */
> > +
> > +static bool
> > +is_sra_scalar_type (tree type)
> > +{
> > +  enum tree_code code = TREE_CODE (type);
> > +  return (code == INTEGER_TYPE || code == REAL_TYPE || code == VECTOR_TYPE
> > +	  || code == COMPLEX_TYPE || code == FIXED_POINT_TYPE
> > +	  || code == ENUMERAL_TYPE || code == BOOLEAN_TYPE
> > +	  || code == POINTER_TYPE || code == OFFSET_TYPE
> > +	  || code == REFERENCE_TYPE);
> Hmm, perhaps better as INTEGERAL_TYPE_P || FOAT_TYPE_P ...
> 
> Also please update those testcases scanning dump files so we don't get
> new regressions.

OK, I have changed this to

static bool
is_sra_scalar_type (tree type)
{
  enum tree_code code = TREE_CODE (type);
  return (INTEGRAL_TYPE_P (type) || FLOAT_TYPE_P (type)
	  || FIXED_POINT_TYPE_P (type) || POINTER_TYPE_P (type)
	  || code == OFFSET_TYPE);
}

Thanks, 


Martin