Re: [PATCH 3/4] New IPA-SRA implementation

[Martin Jambor <mjambor at suse dot cz>]

Hi,

On Fri, Sep 13 2019, Jan Hubicka wrote:
>> This patch actually adds the analysis bits of IPA-SRA - both the
>> function summary generation and the interprocedural analysis and
>> decision stage.  The transformation itself then happens in the call
>> graph cloning infrastructure changes which are in the previous patch.
>> Please see the cover letter of the whole patch-set for more
>> information.
>> 
>> This is mostly only a rebase on the current trunk of the earlier
>> submission, the only functional change is that the pass does not clone
>> when all the work (unused parameter removal) has already been done by
>> IPA-CP.
>> 
>> Martin
>> 
>> 2019-08-20  Martin Jambor  <mjambor@suse.cz>
>> 
>>         * Makefile.in (GTFILES): Added ipa-sra.c.
>>         (OBJS): Added ipa-sra.o.
>>         * dbgcnt.def: Added ipa_sra_params and ipa_sra_retvalues.
>>         * doc/invoke.texi (ipa-sra-max-replacements): New.
>>         * ipa-sra.c: New file.
>>         * lto-section-in.c (lto_section_name): Added ipa-sra section.
>>         * lto-streamer.h (lto_section_type): Likewise.
>>         * params.def (PARAM_IPA_SRA_MAX_REPLACEMENTS): New.
>>         * passes.def: Add new IPA-SRA.
>>         * tree-pass.h (make_pass_ipa_sra): Declare.
> OK

Thanks!

>> +#define IPA_SRA_MAX_PARAM_FLOW_LEN 7
> I would move this to the place ISRA_ARG_SIZE_LIMIT_BITS is defined so
> they appaer at same place.  Perhaps C++ way would be to use constant
> member variable?

OK

...

>> +
>> +/* Quick mapping from a decl to its param descriptor.  */
>> +
>> +static hash_map<tree, gensum_param_desc *> *decl2desc;
>> +
>> +/* Countdown of allowe Alias analysis steps during summary building.  */
>> +
>> +static int aa_walking_limit;
>> +
>> +/* This is a table in which for each basic block and parameter there is a
>> +   distance (offset + size) in that parameter which is dereferenced and
>> +   accessed in that BB.  */
>> +static HOST_WIDE_INT *bb_dereferences = NULL;
>> +/* How many by-reference parameters there are in the current function.  */
>> +static int by_ref_count;
>> +
>> +/* Bitmap of BBs that can cause the function to "stop" progressing by
>> +   returning, throwing externally, looping infinitely or calling a function
>> +   which might abort etc.. */
>> +static bitmap final_bbs;
>> +
>> +/* Obstack to allocate various small structures required only when generating
>> +   summary for a function.  */
>> +static struct obstack gensum_obstack;
>
> Perhaps place the static vars at the beginig of the namespace.
> I think "static" should not be used when declaring vars in a namespace.


OK, I moved them to the beginning of the namespace and removed the
static modifier from them.



>> +/* Scan body function described by NODE and FUN and create access trees for
>> +   parameters.  */
>> +
>> +static void
>> +scan_function (cgraph_node *node, struct function *fun)
>> +{
>> +  basic_block bb;
>> +
>> +  FOR_EACH_BB_FN (bb, fun)
>> +    {
>> +      gimple_stmt_iterator gsi;
>> +      for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
>> +	{
>> +	  gimple *stmt = gsi_stmt (gsi);
>> +
>> +	  if (stmt_can_throw_external (fun, stmt))
>> +	    bitmap_set_bit (final_bbs, bb->index);
>> +	  switch (gimple_code (stmt))
>> +	    {
>> +	    case GIMPLE_RETURN:
>> +	      {
>> +		tree t = gimple_return_retval (as_a <greturn *> (stmt));
>> +		if (t != NULL_TREE)
>> +		  scan_expr_access (t, stmt, ISRA_CTX_LOAD, bb);
>> +		bitmap_set_bit (final_bbs, bb->index);
>> +	      }
>> +	      break;
>> +
>> +	    case GIMPLE_ASSIGN:
>> +	      if (gimple_assign_single_p (stmt)
>> +		  && !gimple_clobber_p (stmt))
>> +		{
>> +		  tree rhs = gimple_assign_rhs1 (stmt);
>> +		  scan_expr_access (rhs, stmt, ISRA_CTX_LOAD, bb);
>> +		  tree lhs = gimple_assign_lhs (stmt);
>> +		  scan_expr_access (lhs, stmt, ISRA_CTX_STORE, bb);
>> +		}
>> +	      break;
>> +
>> +	    case GIMPLE_CALL:
>> +	      {
>> +		unsigned argument_count = gimple_call_num_args (stmt);
>> +		scan_call_info call_info;
>> +		call_info.cs = node->get_edge (stmt);
>> +		call_info.argument_count = argument_count;
>> +
>> +		for (unsigned i = 0; i < argument_count; i++)
>> +		  {
>> +		    call_info.arg_idx = i;
>> +		    scan_expr_access (gimple_call_arg (stmt, i), stmt,
>> +				      ISRA_CTX_ARG, bb, &call_info);
>> +		  }
>> +
>> +		tree lhs = gimple_call_lhs (stmt);
>> +		if (lhs)
>> +		  scan_expr_access (lhs, stmt, ISRA_CTX_STORE, bb);
>> +		int flags = gimple_call_flags (stmt);
>> +		if ((flags & (ECF_CONST | ECF_PURE)) == 0)
>> +		  bitmap_set_bit (final_bbs, bb->index);
>> +	      }
>> +	      break;
>> +
>> +	    case GIMPLE_ASM:
>> +	      {
>> +		gasm *asm_stmt = as_a <gasm *> (stmt);
>> +		walk_stmt_load_store_addr_ops (asm_stmt, NULL, NULL, NULL,
>> +					       asm_visit_addr);
>> +		bitmap_set_bit (final_bbs, bb->index);
>> +
>> +		for (unsigned i = 0; i < gimple_asm_ninputs (asm_stmt); i++)
>> +		  {
>> +		    tree t = TREE_VALUE (gimple_asm_input_op (asm_stmt, i));
>> +		    scan_expr_access (t, stmt, ISRA_CTX_LOAD, bb);
>> +		  }
>> +		for (unsigned i = 0; i < gimple_asm_noutputs (asm_stmt); i++)
>> +		  {
>> +		    tree t = TREE_VALUE (gimple_asm_output_op (asm_stmt, i));
>> +		    scan_expr_access (t, stmt, ISRA_CTX_STORE, bb);
>> +		  }
>> +	      }
>
> Can't this be done by walk_stmt_load_store_addr_ops?

The ASM statement parsing could, I can pretend a followup patch if you'd
like me to.  For the rest, I have to differentiate between normal
non-call loads (ISRA_CTX_LOAD) and loads into call arguments
(ISRA_CTX_ARG


>> +/* Write intraproceural analysis information about NODE and all of its outgoing
>> +   edges into a stream for LTO WPA.  */
>> +
>> +static void
>> +isra_write_node_summary (output_block *ob, cgraph_node *node)
>> +{
>> +  isra_func_summary *ifs = func_sums->get (node);
>> +  lto_symtab_encoder_t encoder = ob->decl_state->symtab_node_encoder;
>> +  int node_ref = lto_symtab_encoder_encode (encoder, node);
>> +  streamer_write_uhwi (ob, node_ref);
>> +
>> +  unsigned param_desc_count = vec_safe_length (ifs->m_parameters);
>> +  streamer_write_uhwi (ob, param_desc_count);
>> +  for (unsigned i = 0; i < param_desc_count; i++)
>> +    {
>> +      isra_param_desc *desc = &(*ifs->m_parameters)[i];
>> +      unsigned access_count = vec_safe_length (desc->accesses);
>> +      streamer_write_uhwi (ob, access_count);
>> +      for (unsigned j = 0; j < access_count; j++)
>> +	{
>> +	  param_access *acc = (*desc->accesses)[j];
>> +	  stream_write_tree (ob, acc->type, true);
>> +	  stream_write_tree (ob, acc->alias_ptr_type, true);
>
> I wonder how things will work when type contains VLA and thus is treamed
> in the local stream?

I hope that cannot happen.  These are the types done to perform memory
access, and ISRA only considers fixed size accesses... so it would have
to be pointers to something with a VLA... which hopefully cannot be used
in interprocedural context in some reasonable way?  If it can, we might
need to traverse the type tree of each candidate and disqualify them.
Do we happen to have a predicate for such types yet?

>
> I believe Richi already commented on the gimple analysis bits.

Somewhat, but not extensively.  Gimple analysis is not really very
different from the old IPA-SRA though.

Thanks,

Martin