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]

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


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.


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