[prev in list] [next in list] [prev in thread] [next in thread] 

List:       gcc-patches
Subject:    Re: PING^3: Re: [patch] implement Cilk Plus simd loops on trunk
From:       Aldy Hernandez <aldyh () redhat ! com>
Date:       2013-09-30 15:18:27
Message-ID: 52499643.20707 () redhat ! com
[Download RAW message or body]

On 09/09/13 07:54, Aldy Hernandez wrote:

PING^3

> Hi guys!
>
> PING for both C and C++.
>
> Thanks.
>
>
> -------- Original Message --------
> Subject: Re: PING: Fwd: Re: [patch] implement Cilk Plus simd loops on trunk
> Date: Tue, 27 Aug 2013 15:03:26 -0500
> From: Aldy Hernandez <aldyh@redhat.com>
> To: Richard Henderson <rth@redhat.com>
> CC: jason merrill <jason@redhat.com>,  gcc-patches
> <gcc-patches@gcc.gnu.org>
>
> On 08/26/13 12:22, Richard Henderson wrote:
>>> +static tree
>>> +c_check_cilk_loop_incr (location_t loc, tree decl, tree incr)
>>> +{
>>> +  if (EXPR_HAS_LOCATION (incr))
>>> +    loc = EXPR_LOCATION (incr);
>>> +
>>> +  if (!incr)
>>> +    {
>>> +      error_at (loc, "missing increment");
>>> +      return error_mark_node;
>>> +    }
>>
>> Either these tests are swapped, or the second one isn't really needed.
>
> Swapped.  Fixed.
>
>>
>>
>>> +  switch (TREE_CODE (incr))
>>> +    {
>>> +    case POSTINCREMENT_EXPR:
>>> +    case PREINCREMENT_EXPR:
>>> +    case POSTDECREMENT_EXPR:
>>> +    case PREDECREMENT_EXPR:
>>> +      if (TREE_OPERAND (incr, 0) != decl)
>>> +    break;
>>> +
>>> +      // Bah... canonicalize into whatever OMP_FOR_INCR needs.
>>> +      if (POINTER_TYPE_P (TREE_TYPE (decl))
>>> +      && TREE_OPERAND (incr, 1))
>>> +    {
>>> +      tree t = fold_convert_loc (loc,
>>> +                     sizetype, TREE_OPERAND (incr, 1));
>>> +
>>> +      if (TREE_CODE (incr) == POSTDECREMENT_EXPR
>>> +          || TREE_CODE (incr) == PREDECREMENT_EXPR)
>>> +        t = fold_build1_loc (loc, NEGATE_EXPR, sizetype, t);
>>> +      t = fold_build_pointer_plus (decl, t);
>>> +      incr = build2 (MODIFY_EXPR, void_type_node, decl, t);
>>> +    }
>>> +      return incr;
>>
>> Handling pointer types and pointer_plus_expr here (p++) ...
>>
>>> +    case MODIFY_EXPR:
>>> +      {
>>> +    tree rhs;
>>> +
>>> +    if (TREE_OPERAND (incr, 0) != decl)
>>> +      break;
>>> +
>>> +    rhs = TREE_OPERAND (incr, 1);
>>> +    if (TREE_CODE (rhs) == PLUS_EXPR
>>> +        && (TREE_OPERAND (rhs, 0) == decl
>>> +        || TREE_OPERAND (rhs, 1) == decl)
>>> +        && INTEGRAL_TYPE_P (TREE_TYPE (rhs)))
>>> +      return incr;
>>> +    else if (TREE_CODE (rhs) == MINUS_EXPR
>>> +         && TREE_OPERAND (rhs, 0) == decl
>>> +         && INTEGRAL_TYPE_P (TREE_TYPE (rhs)))
>>> +      return incr;
>>> +    // Otherwise fail because only PLUS_EXPR and MINUS_EXPR are
>>> +    // allowed.
>>> +    break;
>>
>> ... but not here (p += 1)?
>
> I should make the code more obvious.  What I'm trying to do is generate
> what the gimplifier for OMP_FOR is expecting.  OMP rewrites pointer
> increment/decrement expressions into a corresponding MODIFY_EXPR.
>
> I have abstracted the OMP code and shared it between both type checks,
> and I have also added an assert in the gimplifier just in case some
> future front-end extension generates OMP_FOR_INCR in a wonky way.
>
>>
>>
>>> +c_validate_cilk_plus_loop (tree *tp, int *walk_subtrees, void *data)
>>> +{
>>> +  if (!tp || !*tp)
>>> +    return NULL_TREE;
>>> +
>>> +  bool *valid = (bool *) data;
>>> +
>>> +  switch (TREE_CODE (*tp))
>>> +    {
>>> +    case CALL_EXPR:
>>> +      {
>>> +    tree fndecl = CALL_EXPR_FN (*tp);
>>> +
>>> +    if (TREE_CODE (fndecl) == ADDR_EXPR)
>>> +      fndecl = TREE_OPERAND (fndecl, 0);
>>> +    if (TREE_CODE (fndecl) == FUNCTION_DECL)
>>> +      {
>>> +        if (setjmp_call_p (fndecl))
>>> +          {
>>> +        error_at (EXPR_LOCATION (*tp),
>>> +              "calls to setjmp are not allowed within loops "
>>> +              "annotated with #pragma simd");
>>> +        *valid = false;
>>> +        *walk_subtrees = 0;
>>> +          }
>>> +      }
>>> +    break;
>>
>> Why bother checking for setjmp?  While I agree it makes little sense,
>> there are
>> plenty of other standard functions which also make no sense to use
>> from within
>> #pragma simd.  What's likely to go wrong with a call to setjmp, as
>> opposed to
>> getcontext, pthread_create, or even printf?
>
> Sigh...the standard specifically disallows setjmp.
>
>>
>>> +  if (DECL_REGISTER (decl))
>>> +    {
>>> +      error_at (loc, "induction variable cannot be declared register");
>>> +      return false;
>>> +    }
>>
>> Why?
>
> The standard :(.
>
>>
>> All of the actual gimple changes look good.  You could commit those
>> now if you
>> like to reduce the size of the next patch.
>
> Ughh...got lazy on this round.  How about I commit the gimple changes
> for the next round?
>
> How does this look?
>
>
>

[prev in list] [next in list] [prev in thread] [next in thread]