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.
>
