Re: [gomp4] rewrite simd clone argument adjustment to avoid regimplification

[Martin Jambor <mjambor at suse dot cz>]

Hi,

On Wed, Nov 06, 2013 at 05:37:03PM -0700, Aldy Hernandez wrote:
> On 11/06/13 15:48, Jakub Jelinek wrote:
> >On Wed, Nov 06, 2013 at 03:24:40PM -0700, Aldy Hernandez wrote:
> >>I have checked the following patch with the attached testcases that
> >>were previously ICEing, and with a handful of handcrafted tests that
> >>I checked manually (array references on lhs and rhs, vectors of
> >>pointers, etc).
> >
> >I'd like Martin to eyeball the ipa changes eventually.
> 
> Indeed!  Likewise for all my previous changes to ipa-prop.[ch] and
> tree-sra.c.

Sorry for the delay.  I'd just like to re-iterate one comment from my
previous email which is that I do not think tree-sra.c should export
any function to the outside world apart from the entry points of the
passes (yes, there is already build_ref_for_offset which I admit is
entirely my creation but that should be moved elswhere too).

So please put sra_ipa_get_adjustment_candidate to ipa-prop.c.  I see
that it requires get_ssa_base_param to be moved there as well but
since IPA-SRA uses it in different places, it would need exporting
too, which would be weird because it does not really do anything with
parameters.  Since the function is so trivial, I would even suggest
introducing another private copy to ipa-prop.c (and leaving the
original without the new parameter).  Alternatively, you can move the
function to tree.c but for that it looks too specialized.

Thanks,

Martin

> 
> >>+  if (!SSA_VAR_P (*tp))
> >>+    {
> >>+      /* Make sure we treat subtrees as a RHS.  This makes sure that
> >>+	 when examining the `*foo' in *foo=x, the `foo' get treated as
> >>+	 a use properly.  */
> >
> >I think it wouldn't hurt to e.g. do
> >	if (TYPE_P (*tp))
> >	  *walk_subtrees = 0;
> >here (and drop ATTRIBUTE_UNUSED above.
> 
> Done.
> 
> >
> >>+      wi->is_lhs = false;
> >>+      wi->val_only = true;
> >>+      return NULL_TREE;
> >>+    }
> >>+  struct modify_stmt_info *info = (struct modify_stmt_info *) wi->info;
> >>+  struct ipa_parm_adjustment *cand
> >>+    = sra_ipa_get_adjustment_candidate (tp, NULL, info->adjustments, true);
> >>+  if (!cand)
> >>+    return NULL_TREE;
> >>+
> >>    tree t = *tp;
> >>+  tree repl = make_ssa_name (create_tmp_var (TREE_TYPE (t), NULL), NULL);
> >
> >Just do
> >   tree repl = make_ssa_name (TREE_TYPE (t), NULL);
> >no need to create underlying vars since 4.8.
> 
> Done.
> 
> >
> >>+  /* We have modified in place; update the SSA operands.  */
> >>+  info->modified = false;
> >
> >So you always set modified to false?  I was expecting you'd set
> >it to true here and defer update_stmt and maybe_clean_eh_stmt etc.
> >to after walk_gimple_op (so, do it only when all the changes on the stmt
> >have been performed).  Plus, when you modify something, there is no need
> >to walk subtrees, so you can just do *walk_subtrees = 0; too.
> 
> Hmmm, good point.  I've moved update_stmt and company to the caller,
> and modified the caller to call regimplify_operands only for
> GIMPLE_RETURN which is the special case.
> 
> Let me know if this is still ok so I can commit.
> 
> Thanks.
> Aldy

> gcc/ChangeLog.gomp
> 	* ipa-prop.h (sra_ipa_get_adjustment_candidate): Protoize.
> 	* omp-low.c (struct modify_stmt_info): New.
> 	(ipa_simd_modify_function_body_ops_1): Remove.
> 	(ipa_simd_modify_stmt_ops): New.
> 	(ipa_simd_modify_function_body_ops): Remove.
> 	(ipa_simd_modify_function_body): Set new callback info.
> 	Remove special casing.  Handle all operators with walk_gimple_op.
> 	* tree-sra.c (get_ssa_base_param): Add new argument.  Use it.
> 	(disqualify_base_of_expr): Pass new argument to
> 	get_ssa_base_param.
> 	(sra_ipa_modify_expr): Abstract candidate search into...
> 	(sra_ipa_get_adjustment_candidate): ...here.
>