On 07/09/2015 11:53 AM, Jakub Jelinek wrote:
> Hi!
>
> On Thu, Jul 09, 2015 at 11:24:44AM -0700, Aldy Hernandez wrote:
>
> Thanks for working on it.
>
>> +	      wide_int offset = wi::neg (addend, &overflow);
>> +	      addend = wide_int_to_tree (TREE_TYPE (addend), offset);
>> +	      if (overflow)
>> +		warning_at (c_parser_peek_token (parser)->location,
>> +			    OPT_Woverflow,
>> +			    "possible overflow in %<depend(sink)%> offset");
>
> possible overflow looks weird.  Shouldn't it complain the same
> as it does if you do:
> int c = - (-2147483648);

Done.

> ?
>
>> --- a/gcc/c/c-typeck.c
>> +++ b/gcc/c/c-typeck.c
>> @@ -12489,6 +12489,11 @@ c_finish_omp_clauses (tree clauses, bool declare_simd)
>>   			  == OMP_CLAUSE_DEPEND_SOURCE);
>>   	      break;
>>   	    }
>> +	  if (OMP_CLAUSE_DEPEND_KIND (c) == OMP_CLAUSE_DEPEND_SINK)
>> +	    {
>> +	      gcc_assert (TREE_CODE (t) == TREE_LIST);
>> +	      break;
>> +	    }
>>   	  if (TREE_CODE (t) == TREE_LIST)
>>   	    {
>>   	      if (handle_omp_array_sections (c))
>
> Won't this ICE if somebody uses depend(sink:) ? or depend(sink:.::) or
> similar garbage?  Make sure you don't create OMP_CLAUSE_DEPEND in that
> case.

I've fixed the parser to avoid creating such clause.

>
>> diff --git a/gcc/gimple-walk.c b/gcc/gimple-walk.c
>> index f0e2c67..ba79977 100644
>> --- a/gcc/gimple-walk.c
>> +++ b/gcc/gimple-walk.c
>> @@ -327,6 +327,10 @@ walk_gimple_op (gimple stmt, walk_tree_fn callback_op,
>>         }
>>         break;
>>
>> +    case GIMPLE_OMP_ORDERED:
>> +      /* Ignore clauses.  */
>> +      break;
>> +
>
> I'm not convinced you don't want to walk the clauses.

Ok, I've done so.

Note that the OMP_CLAUSE_DECL will contain a TREE_LIST whose 
TREE_PURPOSE had the variable.  I noticed that walking TREE_LIST's just 
walks the TREE_VALUE, not the TREE_PURPOSE:

     case TREE_LIST:
       WALK_SUBTREE (TREE_VALUE (*tp));
       WALK_SUBTREE_TAIL (TREE_CHAIN (*tp));
       break;


So, I changed the layout of the OMP_CLAUSE_DECL TREE_LIST to have the 
variable in the TREE_VALUE.  The TREE_PURPOSE will contain the lone 
integer, which shouldn't need to be walked.  However, if later (C++ 
iterators??) we have a TREE_PURPOSE that needs to be walked we will have 
to change the walker or the layout.

>
>> diff --git a/gcc/gimple.h b/gcc/gimple.h
>> index 6057ea0..e33fe1e 100644
>> --- a/gcc/gimple.h
>> +++ b/gcc/gimple.h
>> @@ -527,6 +527,17 @@ struct GTY((tag("GSS_OMP_CRITICAL")))
>>     tree name;
>>   };
>>
>> +/* GIMPLE_OMP_ORDERED */
>> +
>> +struct GTY((tag("GSS_OMP_ORDERED")))
>> +  gomp_ordered : public gimple_statement_omp
>> +{
>> +  /* [ WORD 1-7 ] : base class */
>> +
>> +  /* [ WORD 8 ]  */
>> +  tree clauses;
>> +};
>
> I would have expected to use
> struct GTY((tag("GSS_OMP_SINGLE_LAYOUT")))
>    gomp_ordered : public gimple_statement_omp_single_layout
> {
>      /* No extra fields; adds invariant:
>           stmt->code == GIMPLE_OMP_ORDERED.  */
> };
> instead (like gomp_single, gomp_teams, ...).

Oh, neat.  I missed that.  Fixed.

>
>> @@ -149,6 +149,9 @@ struct gimplify_omp_ctx
>>     struct gimplify_omp_ctx *outer_context;
>>     splay_tree variables;
>>     hash_set<tree> *privatized_types;
>> +  /* Iteration variables in an OMP_FOR.  */
>> +  tree *iter_vars;
>> +  int niter_vars;
>
> Wonder if it wouldn't be better to use a vec<tree> instead.
> Then the size would be there as vec_length.

Done.

>
>> @@ -8169,6 +8185,19 @@ gimplify_transaction (tree *expr_p, gimple_seq *pre_p)
>>     return GS_ALL_DONE;
>>   }
>>
>> +/* Verify the validity of the depend(sink:...) variable VAR.
>> +   Return TRUE if everything is OK, otherwise return FALSE.  */
>> +
>> +static bool
>> +verify_sink_var (location_t loc, tree var)
>> +{
>> +  for (int i = 0; i < gimplify_omp_ctxp->niter_vars; ++i)
>> +    if (var == gimplify_omp_ctxp->iter_vars[i])
>> +      return true;
>> +  error_at (loc, "variable %qE is not an iteration variable", var);
>> +  return false;
>
> I believe what we want to verify is that ith variable in the OMP_CLAUSE_DECL
> vector is iter_vars[i], so not just some random permutation etc.

Fixed.

>
>> @@ -3216,7 +3218,51 @@ check_omp_nesting_restrictions (gimple stmt, omp_context *ctx)
>>   	    break;
>>   	  }
>>         break;
>> +    case GIMPLE_OMP_TASK:
>> +      for (c = gimple_omp_task_clauses (stmt); c; c = OMP_CLAUSE_CHAIN (c))
>> +	if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_DEPEND
>> +	    && (OMP_CLAUSE_DEPEND_KIND (c) == OMP_CLAUSE_DEPEND_SOURCE
>> +		|| OMP_CLAUSE_DEPEND_KIND (c) == OMP_CLAUSE_DEPEND_SINK))
>> +	  {
>> +	    error_at (OMP_CLAUSE_LOCATION (c),
>> +		      "depend(%s) is only available in 'omp ordered'",
>
> Please avoid using ' in diagnostics, it should be %<omp ordered%> instead.

Fixed.

>
>> +		      OMP_CLAUSE_DEPEND_KIND (c) == OMP_CLAUSE_DEPEND_SOURCE
>> +		      ? "source" : "sink");
>> +	    return false;
>> +	  }
>> +      break;
>
> This will eventually be needed also for GIMPLE_OMP_TARGET and
> GIMPLE_OMP_ENTER/EXIT_DATA.  But as that isn't really supported right now,
> can wait.

I added an assert so we don't forget.

>
>>       case GIMPLE_OMP_ORDERED:
>> +      for (c = gimple_omp_ordered_clauses (as_a <gomp_ordered *> (stmt));
>> +	   c; c = OMP_CLAUSE_CHAIN (c))
>> +	{
>> +	  enum omp_clause_depend_kind kind = OMP_CLAUSE_DEPEND_KIND (c);
>> +	  if (kind == OMP_CLAUSE_DEPEND_SOURCE
>> +	      || kind == OMP_CLAUSE_DEPEND_SINK)
>> +	    {
>> +	      bool have_ordered = false;
>> +	      /* Look for containing ordered(N) loop.  */
>> +	      for (omp_context *ctx_ = ctx; ctx_; ctx_ = ctx_->outer)
>
> Please use octx or something similar, I don't like the trailing _ ;)

I hate it too, but check_omp_nesting_restrictions() already had a use of 
ctx_ so I followed suit.  Fixed in my code nevertheless.

>
>> +	      if (!have_ordered)
>> +		{
>> +		  error_at (OMP_CLAUSE_LOCATION (c),
>> +			    "depend clause is not within an ordered loop");
>
> Not within is not the right OpenMP term, the requirement is that it must
> be closely nested in ordered loop.

Done.

>
>> +/* depend(sink...) is allowed without an offset.  */
>> +#pragma omp ordered depend(sink:i,j+1)
>
> Can you write depend(sink:i,j-1) at least?  The iteration to depend
> on must be lexicographically earlier in the loop.

Sure.  Neither j+99 or j-HUGE are checked.  We allow anything 
INTEGER_CST.  Perhaps at expansion we can check the sanity of this? 
(Later, when we figure out what we're going to emit for the runtime).

>
>> +#pragma omp ordered depend(sink:i+2,j-2,k+2) /* { dg-error "is not an iteration var" } */
>
> Similarly.  i-2 will be enough.
>
>> --- a/gcc/tree-inline.c
>> +++ b/gcc/tree-inline.c
>> @@ -1479,7 +1479,7 @@ remap_gimple_stmt (gimple stmt, copy_body_data *id)
>>
>>   	case GIMPLE_OMP_ORDERED:
>>   	  s1 = remap_gimple_seq (gimple_omp_body (stmt), id);
>> -	  copy = gimple_build_omp_ordered (s1);
>> +	  copy = gimple_build_omp_ordered (s1, NULL);
>
> You surely don't want to pass NULL here, I bet you want
> gimple_omp_ordered_clauses (stmt) instead.

Fixed.

>
>> --- a/gcc/tree-pretty-print.c
>> +++ b/gcc/tree-pretty-print.c
>> @@ -533,6 +533,9 @@ dump_omp_clause (pretty_printer *pp, tree clause, int spc, int flags)
>>   	case OMP_CLAUSE_DEPEND_SOURCE:
>>   	  pp_string (pp, "source)");
>>   	  return;
>> +	case OMP_CLAUSE_DEPEND_SINK:
>> +	  pp_string (pp, "sink");
>> +	  break;
>
> And here you surely don't want to emit
> #pragma omp ordered(sink
> (note even the missing closing paren).
> It should dump the TREE_LIST (the var and if non-0 addend, the addend after
> it).

Notice this case had a break, not a return, so we would fall down to 
code that printed the TREE_LIST and added a closing parenthesis.  The 
TREE_LIST was in the form of "i 3", which I thought was obvious enough. 
  Be that as it may, I have added code to beautify it as "i+3" as suggested.

OK for branch?