This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

[PATCH] OpenMP #pragma omp declare simd support (take 2)


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)?

> 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).
> 
> > +	  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.
> 
> 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?

	Jakub

Attachment: V033e
Description: Text document

Attachment: V036e
Description: Text document


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]