On Mon, 25 Nov 2013, Jakub Jelinek wrote:
> On Fri, Nov 22, 2013 at 11:08:41AM +0100, Richard Biener wrote:
> > The expr.c hunk is also ok independently of the patch.
>
> This is committed now.
>
> > Ah, here is the stuff moved from. I suppose the IPA param re-org
> > is ok for trunk separately as well.
>
> And this too (without the simdlen field of the adjustment, which turned out
> to be unnecessary).
>
> > What's the reason you cannot defer SIMD cloning to LTRANS stage
> > as simple IPA pass next to IPA-PTA?
>
> Ok, deferring till after IPA-PTA was easy, just small ipa-cp.c changes
> (look at the attribute rather than simd*clone* fields), passes.def and
> had to tweak ipa_add_new_function which assumed that all new functions
> must be definitions with gimple body.
>
> > > + /* Ignore #pragma omp declare simd functions
> > > + if they don't have data references in the
> > > + call stmt itself. */
> > > + if (j == n
> > > + && !(op
> > > + && (DECL_P (op)
> > > + || (REFERENCE_CLASS_P (op)
> > > + && get_base_address (op)))))
> > > + continue;
> >
> > Hmm. I guess I have an idea now how to "better" support calls in
> > data-ref/dependence analysis. The above is fine for now - you
> > might want to dump sth here if you fail because datarefs in a declare
> > simd fn call.
>
> Haven't added any dump here, because there is the:
> >
> > > + }
> > > + }
> > > + }
> > > + LOOP_VINFO_DATAREFS (loop_vinfo) = datarefs;
> > > + if (dump_enabled_p ())
> > > + dump_printf_loc (MSG_MISSED_OPTIMIZATION, vect_location,
> > > + "not vectorized: loop contains function "
> > > + "calls or data references that cannot "
> > > + "be analyzed\n");
>
> which is dumped in that case. Would another message be useful before that
> (or instead of that)?
Hmm, not sure - we can leave it as-is.
> > I'd have expected an unconditional continue here (and leave
> > STMT_VINFO_VECTYPE == NULL - fact is that the vector type of
> > the argument is determined by its definition and thus may
> > be different from what you record here anyway).
>
> Ok, now using STMT_VINFO_VECTYPE = NULL.
>
> > > + if (thisarginfo.vectype != NULL_TREE
> > > + && loop_vinfo
> > > + && TREE_CODE (op) == SSA_NAME
> > > + && simple_iv (loop, loop_containing_stmt (stmt), op, &iv, false)
> > > + && tree_fits_shwi_p (iv.step))
> > > + {
> > > + thisarginfo.linear_step = tree_to_shwi (iv.step);
> >
> > Hmm, you should check thisarginfo.dt instead (I assume this case
> > is for induction/reduction defs)? In this case you also should
> > use STMT_VINFO_LOOP_PHI_EVOLUTION_PART and not re-analyze via simple_iv.
>
> As discussed on IRC, STMT_VINFO_LOOP_PHI_EVOLUTION_PART can't be used,
> because it can be arbitrary linear function argument, not just an IV itself.
> vect-simd-clone-11.c testcase contains examples. This patch doesn't avoid
> calling simple_iv again during transform phase, I don't have a failing
> testcase for that yet (but filed PR59288 for the preexisting issue).
You'll eventually run into a testcase - I promise ;) Anyway, the
solution will be to store the result somewhere (in stmt_vinfo) and
re-use it. We can fix this with a followup (see my fix for PR59288).
> >
> > > + thisarginfo.op = iv.base;
> > > + }
> > > + else if (thisarginfo.vectype == NULL_TREE
> > > + && POINTER_TYPE_P (TREE_TYPE (op)))
> > > + thisarginfo.align = get_pointer_alignment (op) / BITS_PER_UNIT;
> >
> > So this is for dt_external defs?
> >
> > Please switch on thisarginfo.dt here - that more naturally explains
> > what you are doing (otherwise this definitely misses a comment).
>
> Done.
>
> > Please save the result from the analysis (selecting the simd clone)
> > in the stmt_vinfo and skip the analysis during transform phase.
>
> Done.
>
> > > + vec_oprnd0
> > > + = build3 (BIT_FIELD_REF, atype, vec_oprnd0,
> > > + build_int_cst (integer_type_node, prec),
> > > + build_int_cst (integer_type_node,
> > > + (m & (k - 1)) * prec));
> >
> > Some helpers to build the tree to select a sub-vector would be nice
> > (I remember seeing this kind of pattern elsewhere).
>
> I've simplified this to use size_int and bitsize_int for the args
> (as e.g. fold-const.c uses to create BIT_FIELD_REFs), but don't see what
> actually could be put into the helper, besides the BIT_FIELD_REF
> build there is nothing common with other spots and the arguments to that
> call also differ a lot.
Hmm, ok.
> >
> > For SINGLE_RHS assigns I prefer gimple_build_assign.
>
> Done everywhere.
>
> > > + /* Update the exception handling table with the vector stmt if
> > > necessary. */
> > > + if (maybe_clean_or_replace_eh_stmt (stmt, *vec_stmt))
> > > + gimple_purge_dead_eh_edges (gimple_bb (stmt));
> >
> > But you've early-outed on throwing stmts? Generally this shouldn't
> > happen.
>
> Removed (also in vectorizable_call).
>
> Attached is updated full patch (of course against current trunk, so the
> expr.c and generic IPA/tree-sra bits already removed from it), plus
> interdiff for the changes I've done today to the patch.
>
> Ok?
Ok.
Thanks,
Richard.
