RFC (gimplify, openmp): PATCH to is_gimple_reg to check DECL_HAS_VALUE_EXPR_P

While working on another issue I noticed that is_gimple_reg was happily 
accepting VAR_DECLs with DECL_VALUE_EXPR even when later gimplification 
would replace them with something that is_gimple_reg doesn't like, 
leading to trouble.  So I've modified is_gimple_reg to check the VALUE_EXPR.

But this breaks a couple of libgomp testcases, namely 
libgomp.c++/member-[45].C.  This seems to be a latent bug in 
gimplify_omp_for:

>       /* If DECL is not a gimple register, create a temporary variable to act
>          as an iteration counter.  This is valid, since DECL cannot be
>          modified in the body of the loop.  Similarly for any iteration vars
>          in simd with collapse > 1 where the iterator vars must be
>          lastprivate.  */
>       if (orig_for_stmt != for_stmt)
>         var = decl;
>       else if (!is_gimple_reg (decl)
>                || (ort == ORT_SIMD
>                    && TREE_VEC_LENGTH (OMP_FOR_INIT (for_stmt)) > 1))
>         {
>           struct gimplify_omp_ctx *ctx = gimplify_omp_ctxp;
>           /* Make sure omp_add_variable is not called on it prematurely.
>              We call it ourselves a few lines later.  */
>           gimplify_omp_ctxp = NULL;
>           var = create_tmp_var (TREE_TYPE (decl), get_name (decl));
>           gimplify_omp_ctxp = ctx;
>           TREE_OPERAND (t, 0) = var;
>
>           gimplify_seq_add_stmt (&for_body, gimple_build_assign (decl, var));

So we update DECL from VAR in the loop body, but we don't update it 
after the last iteration, when we do the increment but then don't enter 
the body.  As a result, this test fails:

>   #pragma omp parallel for lastprivate (T<Q>::t)
>   for (T<Q>::t = 0; T<Q>::t < 32; T<Q>::t += 3)
>     f[T<Q>::t + 2] |= 2;
>   if (T<Q>::t != 33)
>     __builtin_abort ();

Here T<Q>::t ends up with the value 30 because it doesn't get the last 
update.

Thoughts?

Jason

On June 10, 2016 9:48:45 PM GMT+02:00, Jason Merrill <jason@redhat.com> wrote:
>While working on another issue I noticed that is_gimple_reg was happily
>
>accepting VAR_DECLs with DECL_VALUE_EXPR even when later gimplification
>
>would replace them with something that is_gimple_reg doesn't like, 
>leading to trouble.  So I've modified is_gimple_reg to check the
>VALUE_EXPR.

Can you instead try rejecting them?  I've run into similar issues lately with is_gimple_val.

Richard.

>But this breaks a couple of libgomp testcases, namely 
>libgomp.c++/member-[45].C.  This seems to be a latent bug in 
>gimplify_omp_for:
>
>>       /* If DECL is not a gimple register, create a temporary
>variable to act
>>          as an iteration counter.  This is valid, since DECL cannot
>be
>>          modified in the body of the loop.  Similarly for any
>iteration vars
>>          in simd with collapse > 1 where the iterator vars must be
>>          lastprivate.  */
>>       if (orig_for_stmt != for_stmt)
>>         var = decl;
>>       else if (!is_gimple_reg (decl)
>>                || (ort == ORT_SIMD
>>                    && TREE_VEC_LENGTH (OMP_FOR_INIT (for_stmt)) > 1))
>>         {
>>           struct gimplify_omp_ctx *ctx = gimplify_omp_ctxp;
>>           /* Make sure omp_add_variable is not called on it
>prematurely.
>>              We call it ourselves a few lines later.  */
>>           gimplify_omp_ctxp = NULL;
>>           var = create_tmp_var (TREE_TYPE (decl), get_name (decl));
>>           gimplify_omp_ctxp = ctx;
>>           TREE_OPERAND (t, 0) = var;
>>
>>           gimplify_seq_add_stmt (&for_body, gimple_build_assign
>(decl, var));
>
>So we update DECL from VAR in the loop body, but we don't update it 
>after the last iteration, when we do the increment but then don't enter
>
>the body.  As a result, this test fails:
>
>>   #pragma omp parallel for lastprivate (T<Q>::t)
>>   for (T<Q>::t = 0; T<Q>::t < 32; T<Q>::t += 3)
>>     f[T<Q>::t + 2] |= 2;
>>   if (T<Q>::t != 33)
>>     __builtin_abort ();
>
>Here T<Q>::t ends up with the value 30 because it doesn't get the last 
>update.
>
>Thoughts?
>
>Jason

On Sat, Jun 11, 2016 at 08:43:06PM +0200, Richard Biener wrote:
> On June 10, 2016 9:48:45 PM GMT+02:00, Jason Merrill <jason@redhat.com> wrote:
> >While working on another issue I noticed that is_gimple_reg was happily
> >
> >accepting VAR_DECLs with DECL_VALUE_EXPR even when later gimplification
> >
> >would replace them with something that is_gimple_reg doesn't like, 
> >leading to trouble.  So I've modified is_gimple_reg to check the
> >VALUE_EXPR.
> 
> Can you instead try rejecting them?  I've run into similar issues lately with is_gimple_val.

I'm afraid that would break OpenMP badly.
During gimplification, outside of OpenMP contexts we always replace decls
for their DECL_VALUE_EXPR, but inside of OpenMP contexts we do it only for
some decls.  In particular, omp_notice_variable returns whether the
DECL_VALUE_EXPR should be temporarily ignored (if it returns true) or not.
If DECL_VALUE_EXPR is temporarily ignored, it is only for a short time,
in particular until the omplower pass, which makes sure that the right thing
is done with it and everything is regimplified.

Anyway, looking at Jason's patch, I'm really surprised it didn't break far
more, it is fine if such an ignored DECL_VALUE_EXPR is considered
is_gimple_reg.  And I have no idea how else to express this in the IL,
the DECL_VALUE_EXPR is often something already the FEs set, and we really
want to replace it with the values in most uses, just can't allow it if we
want to replace it by something different instead (e.g. privatize in some
OpenMP/OpenACC region).

	Jakub

On Sat, Jun 11, 2016 at 9:30 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Sat, Jun 11, 2016 at 08:43:06PM +0200, Richard Biener wrote:
>> On June 10, 2016 9:48:45 PM GMT+02:00, Jason Merrill <jason@redhat.com> wrote:
>> >While working on another issue I noticed that is_gimple_reg was happily
>> >
>> >accepting VAR_DECLs with DECL_VALUE_EXPR even when later gimplification
>> >
>> >would replace them with something that is_gimple_reg doesn't like,
>> >leading to trouble.  So I've modified is_gimple_reg to check the
>> >VALUE_EXPR.
>>
>> Can you instead try rejecting them?  I've run into similar issues lately with is_gimple_val.
>
> I'm afraid that would break OpenMP badly.
> During gimplification, outside of OpenMP contexts we always replace decls
> for their DECL_VALUE_EXPR, but inside of OpenMP contexts we do it only for
> some decls.  In particular, omp_notice_variable returns whether the
> DECL_VALUE_EXPR should be temporarily ignored (if it returns true) or not.
> If DECL_VALUE_EXPR is temporarily ignored, it is only for a short time,
> in particular until the omplower pass, which makes sure that the right thing
> is done with it and everything is regimplified.

Ugh :/  Feels like OMP lowering should happen during gimplification then.
The PR71104 fix (yes, still pending...) runs into this generally with the
change to first gimplify the RHS and then the LHS for assignments
as it affects how rhs_predicate_for works - I've adjusted rhs_predicate_for like

@@ -3771,7 +3771,9 @@ gimplify_init_ctor_eval (tree object, ve
 gimple_predicate
 rhs_predicate_for (tree lhs)
 {
-  if (is_gimple_reg (lhs))
+  if (is_gimple_reg (lhs)
+      && (! DECL_P (lhs)
+         || ! DECL_HAS_VALUE_EXPR_P (lhs)))
     return is_gimple_reg_rhs_or_call;
   else
     return is_gimple_mem_rhs_or_call;

but I don't like this very much either (it's Jasons change but rejecting
decls with value expr instead).

Richard.

> Anyway, looking at Jason's patch, I'm really surprised it didn't break far
> more, it is fine if such an ignored DECL_VALUE_EXPR is considered
> is_gimple_reg.  And I have no idea how else to express this in the IL,
> the DECL_VALUE_EXPR is often something already the FEs set, and we really
> want to replace it with the values in most uses, just can't allow it if we
> want to replace it by something different instead (e.g. privatize in some
> OpenMP/OpenACC region).
>
>         Jakub

On Mon, Jun 13, 2016 at 11:03:54AM +0200, Richard Biener wrote:
> > I'm afraid that would break OpenMP badly.
> > During gimplification, outside of OpenMP contexts we always replace decls
> > for their DECL_VALUE_EXPR, but inside of OpenMP contexts we do it only for
> > some decls.  In particular, omp_notice_variable returns whether the
> > DECL_VALUE_EXPR should be temporarily ignored (if it returns true) or not.
> > If DECL_VALUE_EXPR is temporarily ignored, it is only for a short time,
> > in particular until the omplower pass, which makes sure that the right thing
> > is done with it and everything is regimplified.
> 
> Ugh :/  Feels like OMP lowering should happen during gimplification then.

That is not really possible.  OMP lowering relies on all the OpenMP clauses
(including implicitly added) to be finalized before it can figure out what
to do.  And to have the OpenMP clauses finalized, you need to gimplify
everything.  So, it is impossible to do it at the same time as
gimplification, it needs to be another pass (whether a separate full pass,
or a "subpass" of the gimplification matters less; though for debugging,
dumps, etc. having it a separate full pass is better; in any case, it needs
another processing of the whole IL, and for the is_gimple_reg case doesn't
change anything, we still need to postpone the DECL_VALUE_EXPR processing
of some decls in certain uses till the second pass or subpass).

	Jakub

On Mon, Jun 13, 2016 at 5:03 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Sat, Jun 11, 2016 at 9:30 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>> On Sat, Jun 11, 2016 at 08:43:06PM +0200, Richard Biener wrote:
>>> On June 10, 2016 9:48:45 PM GMT+02:00, Jason Merrill <jason@redhat.com> wrote:
>>> >While working on another issue I noticed that is_gimple_reg was happily
>>> >
>>> >accepting VAR_DECLs with DECL_VALUE_EXPR even when later gimplification
>>> >
>>> >would replace them with something that is_gimple_reg doesn't like,
>>> >leading to trouble.  So I've modified is_gimple_reg to check the
>>> >VALUE_EXPR.
>>>
>>> Can you instead try rejecting them?  I've run into similar issues lately with is_gimple_val.
>>
>> I'm afraid that would break OpenMP badly.
>> During gimplification, outside of OpenMP contexts we always replace decls
>> for their DECL_VALUE_EXPR, but inside of OpenMP contexts we do it only for
>> some decls.  In particular, omp_notice_variable returns whether the
>> DECL_VALUE_EXPR should be temporarily ignored (if it returns true) or not.
>> If DECL_VALUE_EXPR is temporarily ignored, it is only for a short time,
>> in particular until the omplower pass, which makes sure that the right thing
>> is done with it and everything is regimplified.
>
> Ugh :/  Feels like OMP lowering should happen during gimplification then.
> The PR71104 fix (yes, still pending...) runs into this generally with the
> change to first gimplify the RHS and then the LHS for assignments

Yep, that's what led me here, too.

Jason

> as it affects how rhs_predicate_for works - I've adjusted rhs_predicate_for like

> @@ -3771,7 +3771,9 @@ gimplify_init_ctor_eval (tree object, ve
>  gimple_predicate
>  rhs_predicate_for (tree lhs)
>  {
> -  if (is_gimple_reg (lhs))
> +  if (is_gimple_reg (lhs)
> +      && (! DECL_P (lhs)
> +         || ! DECL_HAS_VALUE_EXPR_P (lhs)))
>      return is_gimple_reg_rhs_or_call;
>    else
>      return is_gimple_mem_rhs_or_call;
>
> but I don't like this very much either (it's Jasons change but rejecting
> decls with value expr instead).
>
> Richard.
>
>> Anyway, looking at Jason's patch, I'm really surprised it didn't break far
>> more, it is fine if such an ignored DECL_VALUE_EXPR is considered
>> is_gimple_reg.  And I have no idea how else to express this in the IL,
>> the DECL_VALUE_EXPR is often something already the FEs set, and we really
>> want to replace it with the values in most uses, just can't allow it if we
>> want to replace it by something different instead (e.g. privatize in some
>> OpenMP/OpenACC region).
>>
>>         Jakub