[PATCH PING] c++-specific bits of tree-slimming patches

[PATCH PING] c++-specific bits of tree-slimming patches

[Nathan Froyd]

The C++-specific bits of these patches:

  [PATCH 02/18] enforce TREE_CHAIN and TREE_TYPE accesses
  http://gcc.gnu.org/ml/gcc-patches/2011-03/msg00557.html

  [PATCH 07/18] generalize build_case_label to the rest of the compiler
  http://gcc.gnu.org/ml/gcc-patches/2011-03/msg00557.html

  [PATCH 08/18] convert cp *FOR_STMTs to use private scope fields
  http://gcc.gnu.org/ml/gcc-patches/2011-03/msg00553.html

  [PATCH 09/18] convert cp IF_STMTs to use private scope fields
  http://gcc.gnu.org/ml/gcc-patches/2011-03/msg00562.html

  [PATCH 10/18] convert cp SWITCH_STMTs to use private scope fields
  http://gcc.gnu.org/ml/gcc-patches/2011-03/msg00552.html

  [PATCH 11/18] mark EXPR_PACK_EXPANSION as typed only
  http://gcc.gnu.org/ml/gcc-patches/2011-03/msg00563.html

  [PATCH 17/18] introduce block_chainon and use BLOCK_CHAIN more
  http://gcc.gnu.org/ml/gcc-patches/2011-03/msg00566.html

are still pending review.

-Nathan

Re: [PATCH PING] c++-specific bits of tree-slimming patches

[Nathan Froyd]

On Thu, Mar 24, 2011 at 06:15:18AM -0700, Nathan Froyd wrote:
> The C++-specific bits of these patches:
> 
>   [PATCH 02/18] enforce TREE_CHAIN and TREE_TYPE accesses
>   http://gcc.gnu.org/ml/gcc-patches/2011-03/msg00557.html
> 
>   [PATCH 07/18] generalize build_case_label to the rest of the compiler
>   http://gcc.gnu.org/ml/gcc-patches/2011-03/msg00557.html
> 
>   [PATCH 08/18] convert cp *FOR_STMTs to use private scope fields
>   http://gcc.gnu.org/ml/gcc-patches/2011-03/msg00553.html
> 
>   [PATCH 09/18] convert cp IF_STMTs to use private scope fields
>   http://gcc.gnu.org/ml/gcc-patches/2011-03/msg00562.html
> 
>   [PATCH 10/18] convert cp SWITCH_STMTs to use private scope fields
>   http://gcc.gnu.org/ml/gcc-patches/2011-03/msg00552.html
> 
>   [PATCH 11/18] mark EXPR_PACK_EXPANSION as typed only
>   http://gcc.gnu.org/ml/gcc-patches/2011-03/msg00563.html
> 
>   [PATCH 17/18] introduce block_chainon and use BLOCK_CHAIN more
>   http://gcc.gnu.org/ml/gcc-patches/2011-03/msg00566.html
> 
> are still pending review.

Ping^2.

Alternatively, could we have a GWP or similar rule on Tom's suggestion:

http://gcc.gnu.org/ml/gcc-patches/2011-03/msg00620.html

that patches propagating middle-end changes to front-ends are
obvious/preapproved, perhaps after a 24-48 hour waiting period for
comments?  That would cover 02 and 07 above (possibly 17 as well); 02 is
blocking some of the already-approved middle-end changes later in the
series.

I think Tom's suggestion makes a lot of sense, but I'm not exactly a
disinterested observer... :)

-Nathan

Re: [PATCH PING] c++-specific bits of tree-slimming patches

[Jason Merrill]

On 03/24/2011 09:15 AM, Nathan Froyd wrote:
> The C++-specific bits of these patches:
>
>    [PATCH 02/18] enforce TREE_CHAIN and TREE_TYPE accesses
>    http://gcc.gnu.org/ml/gcc-patches/2011-03/msg00557.html

OK.

>    [PATCH 07/18] generalize build_case_label to the rest of the compiler
>    http://gcc.gnu.org/ml/gcc-patches/2011-03/msg00557.html

> +  tree t = make_node (CASE_LABEL_EXPR);
> +
> +  TREE_TYPE (t) = void_type_node;
> +  SET_EXPR_LOCATION (t, input_location);

As jsm and richi said, using input_location like this is a regression. 
Can we use DECL_SOURCE_LOCATION (label_decl) instead?

>    [PATCH 08/18] convert cp *FOR_STMTs to use private scope fields
>    http://gcc.gnu.org/ml/gcc-patches/2011-03/msg00553.html
>    [PATCH 09/18] convert cp IF_STMTs to use private scope fields
>    http://gcc.gnu.org/ml/gcc-patches/2011-03/msg00562.html
>    [PATCH 10/18] convert cp SWITCH_STMTs to use private scope fields
>    http://gcc.gnu.org/ml/gcc-patches/2011-03/msg00552.html

OK.

>    [PATCH 11/18] mark EXPR_PACK_EXPANSION as typed only
>    http://gcc.gnu.org/ml/gcc-patches/2011-03/msg00563.html

It looks like you need to add EXPR_PACK_EXPANSION cases to 
value_dependent_expression_p and cp_tree_equal.  Maybe split out the 
code from write_expression that overrides TREE_OPERAND_LENGTH in some 
cases and use that new function instead of TREE_OPERAND_LENGTH in these 
places.

>    [PATCH 17/18] introduce block_chainon and use BLOCK_CHAIN more
>    http://gcc.gnu.org/ml/gcc-patches/2011-03/msg00566.html

OK.

> Alternatively, could we have a GWP or similar rule on Tom's suggestion:
>
> http://gcc.gnu.org/ml/gcc-patches/2011-03/msg00620.html
>
> that patches propagating middle-end changes to front-ends are
> obvious/preapproved, perhaps after a 24-48 hour waiting period for
> comments?  That would cover 02 and 07 above (possibly 17 as well); 02 is
> blocking some of the already-approved middle-end changes later in the
> series.

That makes sense to me, but I'd give it a week.

Jason

Re: [PATCH PING] c++-specific bits of tree-slimming patches

[Nathan Froyd]

On Fri, Apr 08, 2011 at 01:50:24PM -0400, Jason Merrill wrote:
> On 03/24/2011 09:15 AM, Nathan Froyd wrote:
>> +  tree t = make_node (CASE_LABEL_EXPR);
>> +
>> +  TREE_TYPE (t) = void_type_node;
>> +  SET_EXPR_LOCATION (t, input_location);
>
> As jsm and richi said, using input_location like this is a regression.  
> Can we use DECL_SOURCE_LOCATION (label_decl) instead?

Sure.  Joseph, Richi, are you happy with that change?  It would fix the
C/C++ regression, as c_add_case_label does:

  /* Create the LABEL_DECL itself.  */
  label = create_artificial_label (loc);
  ...
  /* Add a CASE_LABEL to the statement-tree.  */
  case_label = add_stmt (build_case_label (loc, low_value, high_value, label));

so the DECL_SOURCE_LOCATION would be the same as the location_t we were
passing in anyway.  For the other languages, I think it would be neutral
or an improvement (they all use input_location or UNKNOWN_LOCATION for
the CASE_LABEL anyway).

>>    [PATCH 11/18] mark EXPR_PACK_EXPANSION as typed only
>>    http://gcc.gnu.org/ml/gcc-patches/2011-03/msg00563.html
>
> It looks like you need to add EXPR_PACK_EXPANSION cases to  
> value_dependent_expression_p and cp_tree_equal.  Maybe split out the  
> code from write_expression that overrides TREE_OPERAND_LENGTH in some  
> cases and use that new function instead of TREE_OPERAND_LENGTH in these  
> places.

Thanks for catching this, will do.

-Nathan

Re: [PATCH PING] c++-specific bits of tree-slimming patches

[Richard Guenther]

On Thu, 14 Apr 2011, Nathan Froyd wrote:

> On Fri, Apr 08, 2011 at 01:50:24PM -0400, Jason Merrill wrote:
> > On 03/24/2011 09:15 AM, Nathan Froyd wrote:
> >> +  tree t = make_node (CASE_LABEL_EXPR);
> >> +
> >> +  TREE_TYPE (t) = void_type_node;
> >> +  SET_EXPR_LOCATION (t, input_location);
> >
> > As jsm and richi said, using input_location like this is a regression.  
> > Can we use DECL_SOURCE_LOCATION (label_decl) instead?
> 
> Sure.  Joseph, Richi, are you happy with that change?  It would fix the
> C/C++ regression, as c_add_case_label does:
> 
>   /* Create the LABEL_DECL itself.  */
>   label = create_artificial_label (loc);
>   ...
>   /* Add a CASE_LABEL to the statement-tree.  */
>   case_label = add_stmt (build_case_label (loc, low_value, high_value, label));
> 
> so the DECL_SOURCE_LOCATION would be the same as the location_t we were
> passing in anyway.  For the other languages, I think it would be neutral
> or an improvement (they all use input_location or UNKNOWN_LOCATION for
> the CASE_LABEL anyway).

Yes, using DECL_SOURCE_LOCATION (label_decl) sounds like the correct
thing.

Richard.

Re: [PATCH PING] c++-specific bits of tree-slimming patches

[Joseph S. Myers]

On Thu, 14 Apr 2011, Nathan Froyd wrote:

> On Fri, Apr 08, 2011 at 01:50:24PM -0400, Jason Merrill wrote:
> > On 03/24/2011 09:15 AM, Nathan Froyd wrote:
> >> +  tree t = make_node (CASE_LABEL_EXPR);
> >> +
> >> +  TREE_TYPE (t) = void_type_node;
> >> +  SET_EXPR_LOCATION (t, input_location);
> >
> > As jsm and richi said, using input_location like this is a regression.  
> > Can we use DECL_SOURCE_LOCATION (label_decl) instead?
> 
> Sure.  Joseph, Richi, are you happy with that change?  It would fix the
> C/C++ regression, as c_add_case_label does:
> 
>   /* Create the LABEL_DECL itself.  */
>   label = create_artificial_label (loc);
>   ...
>   /* Add a CASE_LABEL to the statement-tree.  */
>   case_label = add_stmt (build_case_label (loc, low_value, high_value, label));
> 
> so the DECL_SOURCE_LOCATION would be the same as the location_t we were
> passing in anyway.  For the other languages, I think it would be neutral
> or an improvement (they all use input_location or UNKNOWN_LOCATION for
> the CASE_LABEL anyway).

Seems fine with me.

-- 
Joseph S. Myers
joseph@codesourcery.com

Re: [PATCH PING] c++-specific bits of tree-slimming patches

[Nathan Froyd]

On Fri, Apr 08, 2011 at 01:50:24PM -0400, Jason Merrill wrote:
> On 03/24/2011 09:15 AM, Nathan Froyd wrote:
>> +  tree t = make_node (CASE_LABEL_EXPR);
>> +
>> +  TREE_TYPE (t) = void_type_node;
>> +  SET_EXPR_LOCATION (t, input_location);
>
> As jsm and richi said, using input_location like this is a regression.  
> Can we use DECL_SOURCE_LOCATION (label_decl) instead?

I went off and tried that; some callers provide a NULL label_decl.
What's the right thing to do in that case--use UNKNOWN_LOCATION or
input_location?  I'm leaning towards the former.

-Nathan

Re: [PATCH PING] c++-specific bits of tree-slimming patches

[Jason Merrill]

On 04/21/2011 08:50 PM, Nathan Froyd wrote:
> On Fri, Apr 08, 2011 at 01:50:24PM -0400, Jason Merrill wrote:
>> As jsm and richi said, using input_location like this is a regression.
>> Can we use DECL_SOURCE_LOCATION (label_decl) instead?
>
> I went off and tried that; some callers provide a NULL label_decl.

Hunh.  How does that work?  They fill in CASE_LABEL later?  Can that be 
changed?

Jason

Re: [PATCH PING] c++-specific bits of tree-slimming patches

[Nathan Froyd]

On Thu, Apr 21, 2011 at 10:49:05PM -0400, Jason Merrill wrote:
> On 04/21/2011 08:50 PM, Nathan Froyd wrote:
>> On Fri, Apr 08, 2011 at 01:50:24PM -0400, Jason Merrill wrote:
>>> As jsm and richi said, using input_location like this is a regression.
>>> Can we use DECL_SOURCE_LOCATION (label_decl) instead?
>>
>> I went off and tried that; some callers provide a NULL label_decl.
>
> Hunh.  How does that work?  They fill in CASE_LABEL later?  Can that be  
> changed?

Yeah, tree-eh.c:lower_try_finally_switch.  I don't know how easy it is
to fix; it certainly looks non-trivial.

-Nathan

Re: [PATCH PING] c++-specific bits of tree-slimming patches

[Jason Merrill]

[-- Attachment #1: Type: text/plain, Size: 538 bytes --]

On 04/21/2011 10:55 PM, Nathan Froyd wrote:
> On Thu, Apr 21, 2011 at 10:49:05PM -0400, Jason Merrill wrote:
>> Hunh.  How does that work?  They fill in CASE_LABEL later?  Can that be
>> changed?
>
> Yeah, tree-eh.c:lower_try_finally_switch.  I don't know how easy it is
> to fix; it certainly looks non-trivial.

Well, I tried adjusting it and regression testing seems fine so far.  I 
can't think what the comment would be talking about with pointers not 
providing a stable order; I don't see anything that would rely on that.

Jason


[-- Attachment #2: case-lab.patch --]
[-- Type: text/plain, Size: 1607 bytes --]

commit 50cd5e483b89a3be6fd9e432edf1ece31f6756bd
Author: Jason Merrill <jason@redhat.com>
Date:   Fri Apr 22 00:47:36 2011 -0400

    bar

diff --git a/gcc/tree-eh.c b/gcc/tree-eh.c
index 60e2236..feee182 100644
--- a/gcc/tree-eh.c
+++ b/gcc/tree-eh.c
@@ -1419,11 +1419,9 @@ lower_try_finally_switch (struct leh_state *state, struct leh_tf_state *tf)
           void **slot;
           case_lab = build3 (CASE_LABEL_EXPR, void_type_node,
                              build_int_cst (NULL, switch_id),
-			     NULL, NULL);
+			     NULL, create_artificial_label (tf_loc));
           /* We store the cont_stmt in the pointer map, so that we can recover
-             it in the loop below.  We don't create the new label while
-             walking the goto_queue because pointers don't offer a stable
-             order.  */
+             it in the loop below.  */
           if (!cont_map)
             cont_map = pointer_map_create ();
           slot = pointer_map_insert (cont_map, case_lab);
@@ -1443,13 +1441,10 @@ lower_try_finally_switch (struct leh_state *state, struct leh_tf_state *tf)
       gcc_assert (cont_map);
 
       slot = pointer_map_contains (cont_map, last_case);
-      /* As the comment above suggests, CASE_LABEL (last_case) was just a
-         placeholder, it does not store an actual label, yet. */
       gcc_assert (slot);
       cont_stmt = *(gimple *) slot;
 
-      label = create_artificial_label (tf_loc);
-      CASE_LABEL (last_case) = label;
+      label = CASE_LABEL (last_case);
 
       x = gimple_build_label (label);
       gimple_seq_add_stmt (&switch_body, x);

Re: [PATCH PING] c++-specific bits of tree-slimming patches

[Mike Stump]

On Apr 21, 2011, at 9:59 PM, Jason Merrill wrote:
> On 04/21/2011 10:55 PM, Nathan Froyd wrote:
>> On Thu, Apr 21, 2011 at 10:49:05PM -0400, Jason Merrill wrote:
>>> Hunh.  How does that work?  They fill in CASE_LABEL later?  Can that be
>>> changed?
>> 
>> Yeah, tree-eh.c:lower_try_finally_switch.  I don't know how easy it is
>> to fix; it certainly looks non-trivial.
> 
> Well, I tried adjusting it and regression testing seems fine so far.

Unsurprising...  It will never fail during testsuite run, and won't always fail during a bootstrap.

> I can't think what the comment would be talking about with pointers not providing a stable order; I don't see anything that would rely on that.

  http://gcc.gnu.org/ml/gcc/2005-04/msg00161.html

has the details of why the code was put in.  Essentially, the Ada boostrap on x86 linux.  What's worse is, at the time, it would only occasionally fail, so, a bootstrap that works won't prove anything.

Re: [PATCH PING] c++-specific bits of tree-slimming patches

[Richard Guenther]

On Fri, Apr 22, 2011 at 8:13 AM, Mike Stump <mikestump@comcast.net> wrote:
> On Apr 21, 2011, at 9:59 PM, Jason Merrill wrote:
>> On 04/21/2011 10:55 PM, Nathan Froyd wrote:
>>> On Thu, Apr 21, 2011 at 10:49:05PM -0400, Jason Merrill wrote:
>>>> Hunh.  How does that work?  They fill in CASE_LABEL later?  Can that be
>>>> changed?
>>>
>>> Yeah, tree-eh.c:lower_try_finally_switch.  I don't know how easy it is
>>> to fix; it certainly looks non-trivial.
>>
>> Well, I tried adjusting it and regression testing seems fine so far.
>
> Unsurprising...  It will never fail during testsuite run, and won't always fail during a bootstrap.
>
>> I can't think what the comment would be talking about with pointers not providing a stable order; I don't see anything that would rely on that.
>
>  http://gcc.gnu.org/ml/gcc/2005-04/msg00161.html
>
> has the details of why the code was put in.  Essentially, the Ada boostrap on x86 linux.  What's worse is, at the time, it would only occasionally fail, so, a bootstrap that works won't prove anything.

Well, unless we are not walking a pointer-based hashtable I don't see
how this matters here.

To Nathan: yes, UNKNOWN_LOCATION would be correct.  Whoever then sets
the label should adjust it accordingly.

Richard.

Re: [PATCH PING] c++-specific bits of tree-slimming patches

[Nathan Froyd]

On Fri, Apr 22, 2011 at 11:12:01AM +0200, Richard Guenther wrote:
> On Fri, Apr 22, 2011 at 8:13 AM, Mike Stump <mikestump@comcast.net> wrote:
> > Unsurprising...  It will never fail during testsuite run, and won't
> > always fail during a bootstrap.
> >
> >> I can't think what the comment would be talking about with pointers
> >> not providing a stable order; I don't see anything that would rely
> >> on that.
> >
> >  http://gcc.gnu.org/ml/gcc/2005-04/msg00161.html
> >
> > has the details of why the code was put in.  Essentially, the Ada
> > boostrap on x86 linux.  What's worse is, at the time, it would only
> > occasionally fail, so, a bootstrap that works won't prove anything.
> 
> Well, unless we are not walking a pointer-based hashtable I don't see
> how this matters here.

I can't see the pointer traversal, either--unless there's some subtlety
in how things are added to the goto_queue.

I'm going to leave the code alone for the moment.

> To Nathan: yes, UNKNOWN_LOCATION would be correct.  Whoever then sets
> the label should adjust it accordingly.

Will commit with that change in build_case_label, then.  I will leave
the location-setting to a separate commit, if any.

-Nathan

Re: [PATCH PING] c++-specific bits of tree-slimming patches

[Jason Merrill]

On 04/22/2011 02:13 AM, Mike Stump wrote:
>    http://gcc.gnu.org/ml/gcc/2005-04/msg00161.html
>
> has the details of why the code was put in.

Right.  At the time, we were sorting the goto queue based on pointer 
values, which caused the problem.  We no longer do that, so we shouldn't 
need this workaround (deferring creation of case label labels) anymore.

What do you think, Alex?

Jason

Re: [PATCH PING] c++-specific bits of tree-slimming patches

[Mike Stump]

On Apr 22, 2011, at 8:55 AM, Jason Merrill wrote:
> On 04/22/2011 02:13 AM, Mike Stump wrote:
>>   http://gcc.gnu.org/ml/gcc/2005-04/msg00161.html
>> 
>> has the details of why the code was put in.
> 
> Right.  At the time, we were sorting the goto queue based on pointer values, which caused the problem.  We no longer do that,

Excellent.  That was the only concern I knew of.

Re: [PATCH PING] c++-specific bits of tree-slimming patches

[Alexandre Oliva]

On Apr 22, 2011, Jason Merrill <jason@redhat.com> wrote:

> On 04/22/2011 02:13 AM, Mike Stump wrote:
>> http://gcc.gnu.org/ml/gcc/2005-04/msg00161.html
>> 
>> has the details of why the code was put in.

> Right.  At the time, we were sorting the goto queue based on pointer
> values, which caused the problem.  We no longer do that, so we
> shouldn't need this workaround (deferring creation of case label
> labels) anymore.

> What do you think, Alex?

Yeah, this change looks safe now.

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist      Red Hat Brazil Compiler Engineer

Re: [PATCH PING] c++-specific bits of tree-slimming patches

[Nathan Froyd]

On Fri, Apr 22, 2011 at 12:59:21AM -0400, Jason Merrill wrote:
> On 04/21/2011 10:55 PM, Nathan Froyd wrote:
>> On Thu, Apr 21, 2011 at 10:49:05PM -0400, Jason Merrill wrote:
>>> Hunh.  How does that work?  They fill in CASE_LABEL later?  Can that be
>>> changed?
>>
>> Yeah, tree-eh.c:lower_try_finally_switch.  I don't know how easy it is
>> to fix; it certainly looks non-trivial.
>
> Well, I tried adjusting it and regression testing seems fine so far.  I  
> can't think what the comment would be talking about with pointers not  
> providing a stable order; I don't see anything that would rely on
> that.

Based on discussion downthread, I plan to commit something like your
patch (I think `label' is unused after this, so requires trivial
changes) on your behalf tomorrow, unless you beat me to it or unless
somebody yells.  I'd rather have this not mixed up with the rest of the
build_case_label changes.

-Nathan