Hi,
On Fri, Nov 22, 2013 at 12:19:33PM +0100, Jakub Jelinek wrote:
> On Fri, Nov 22, 2013 at 11:08:41AM +0100, Richard Biener wrote:
> > > @@ -284,6 +382,12 @@ public:
> > > /* Declaration node used to be clone of. */
> > > tree former_clone_of;
> > >
> > > + /* If this is a SIMD clone, this points to the SIMD specific
> > > + information for it. */
> > > + struct cgraph_simd_clone *simdclone;
> > > + /* If this function has SIMD clones, this points to the first clone.
> > > */
> > > + struct cgraph_node *simd_clones;
> > > +
> >
> > I wonder how you run all of this through LTO (I'll see below I guess ;))
>
> It doesn't work, as in, all the added testcases work just fine without -flto
> and all of them ICE with -flto, but there are multiple known issues with LTO
> before that (internal fns, etc.). More below.
>
> > The expr.c hunk is also ok independently of the patch.
>
> Ok, thanks (though without the rest of the patch probably nothing emits it).
>
> > > @@ -3758,6 +3772,124 @@ ipa_modify_call_arguments (struct cgraph
> > > free_dominance_info (CDI_DOMINATORS);
> > > }
> >
> > You've run the above through Martin IIRC, but ...
>
> Aldy did.
>
> > > +/* If the expression *EXPR should be replaced by a reduction of a
> > > parameter, do
> > > + so. ADJUSTMENTS is a pointer to a vector of adjustments. CONVERT
> > > + specifies whether the function should care about type incompatibility
> > > the
> > > + current and new expressions. If it is false, the function will leave
> > > + incompatibility issues to the caller. Return true iff the expression
> > > + was modified. */
> > > +
> > > +bool
> > > +ipa_modify_expr (tree *expr, bool convert,
> > > + ipa_parm_adjustment_vec adjustments)
> > > +{
> > > + struct ipa_parm_adjustment *cand
> > > + = ipa_get_adjustment_candidate (&expr, &convert, adjustments, false);
> > > + if (!cand)
> > > + return false;
> > > +
> > > + tree src;
> > > + if (cand->by_ref)
> > > + src = build_simple_mem_ref (cand->new_decl);
> >
> > is this function mostly copied from elsewhere? Because
> > using build_simple_mem_ref always smells like possible TBAA problems.
>
> Perhaps, but this is just code reorg, the same
>
> - if (cand->by_ref)
> - src = build_simple_mem_ref (cand->reduction);
> - else
> - src = cand->reduction;
>
> used to sit in sra_ipa_modify_expr before.
IPA-SRA (in splice_param_accesses) makes sure that it only pushes
dereferences to the caller (which are created by this code) when all
dereferences in the calle have the same reference_alias_ptr_type. THe
dereference is then made in type of one such dereference. I hope that
means there are no TBAA issues.
>
> >
> > > + else
> > > + src = cand->new_decl;
> > > +
> > > + if (dump_file && (dump_flags & TDF_DETAILS))
> > > + {
> > > + fprintf (dump_file, "About to replace expr ");
> > > + print_generic_expr (dump_file, *expr, 0);
> > > + fprintf (dump_file, " with ");
> > > + print_generic_expr (dump_file, src, 0);
> > > + fprintf (dump_file, "\n");
> > > + }
> > > +
> > > + if (convert && !useless_type_conversion_p (TREE_TYPE (*expr),
> > > cand->type))
> > > + {
> > > + tree vce = build1 (VIEW_CONVERT_EXPR, TREE_TYPE (*expr), src);
> > > + *expr = vce;
> >
> > Why build1 and not fold it? I assume from above you either have a plain
> > decl (cand->new_decl) or a MEM_REF. For both cases simply folding
> > the VCE into a MEM_REF works.
>
> Again, preexisting code from sra_ipa_modify_expr. Can it be changed
> incrementally/independently of this?
I'm not sure, perhaps it made sense before we had MEM_REF and it was
not converted... or it was simply always a bug. I can fix this after
the merge, not sure whether now of in the next stage1 though.
>
> > > + }
> > > + else
> > > + *expr = src;
> > > + return true;
> > > +}
> > > +
> > > +/* If T is an SSA_NAME, return NULL if it is not a default def or
> > > + return its base variable if it is. If IGNORE_DEFAULT_DEF is true,
> > > + the base variable is always returned, regardless if it is a default
> > > + def. Return T if it is not an SSA_NAME. */
> > > +
> > > +static tree
> > > +get_ssa_base_param (tree t, bool ignore_default_def)
> > > +{
> > > + if (TREE_CODE (t) == SSA_NAME)
> > > + {
> > > + if (ignore_default_def || SSA_NAME_IS_DEFAULT_DEF (t))
> > > + return SSA_NAME_VAR (t);
> > > + else
> > > + return NULL_TREE;
> > > + }
> > > + return t;
> > > +}
> >
> > This function will return non-NULL for non-PARMs - is that intended?
>
> Again, seems to be preexisting code from tree-sra.c. Aldy/Martin?
Yeah, at least in the old form it is either checked later on or it
does not matter (we just use the DECL_UID to clear its bit in bitmap
of candidates). But it would probaly make sense to move the check
there, again as a followup, whether now or for 4.10.
Thanks,
Martin
