diff mbox

[3/5] Handle original loop tree in expand_omp_for_generic

Message ID 561BE63D.2030907@mentor.com
State New
Headers show

Commit Message

Tom de Vries Oct. 12, 2015, 4:56 p.m. UTC 
On 12/10/15 16:11, Bernd Schmidt wrote:
> On 10/10/2015 01:58 PM, Tom de Vries wrote:
>
>> Handle original loop tree in expand_omp_for_generic
>>
>> 2015-09-10  Tom de Vries<tom@codesourcery.com>
>>
>>     PR tree-optimization/67476
>>     * omp-low.c (expand_omp_for_generic): Handle original loop tree.
>
> This one I find slightly confusing.
>
>> -      add_bb_to_loop (l2_bb, cont_bb->loop_father);
>> +      struct loop *loop = l1_bb->loop_father;
>> +      add_bb_to_loop (l2_bb, entry_bb->loop_father);
>>         add_loop (outer_loop, l0_bb->loop_father);
>
> Looks like a lot of bb's loop_father is being looked at. Are all or some
> of these supposed to be the same? I think I'd like one (appropriately
> named) struct loop * variable for each loop that's involved here.

Done.

> There's a comment suggesting that there can be different situations, it
> would be good to expand that to explain how they can arise.
>
>> -      struct loop *loop = alloc_loop ();
>> +      loop = alloc_loop ();
>
> Also, I think it would be preferrable to not reuse that loop variable
> but make a new one instead.
>

Does this version look better?

Thanks,
- Tom

Comments

Bernd Schmidt Oct. 12, 2015, 5:24 p.m. UTC | #1 
> Does this version look better?

In terms of clarity, yes. Only one thing:

> +      if (/* If we already have a loop struct for the original loop, don't
> +	     allocate a new one.  */
> +	  !orig_loop_has_loop_struct

Don't really like the formatting with this comment. I'd pull it in front 
of the if statement, and change it to
  /* Allocate a loop structure for the original loop unless we already
     had one.  */

Ok with that change.


Bernd
diff mbox

Patch

Handle original loop tree in expand_omp_for_generic

2015-09-12  Tom de Vries  <tom@codesourcery.com>

	PR tree-optimization/67476
	* omp-low.c (expand_omp_for_generic): Handle original loop tree.
---
 gcc/omp-low.c | 30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/gcc/omp-low.c b/gcc/omp-low.c
index b2a93b9..b957428 100644
--- a/gcc/omp-low.c
+++ b/gcc/omp-low.c
@@ -6439,7 +6439,6 @@  expand_omp_for_generic (struct omp_region *region,
       remove_edge (e);
 
       make_edge (cont_bb, l2_bb, EDGE_FALSE_VALUE);
-      add_bb_to_loop (l2_bb, cont_bb->loop_father);
       e = find_edge (cont_bb, l1_bb);
       if (e == NULL)
 	{
@@ -6516,17 +6515,30 @@  expand_omp_for_generic (struct omp_region *region,
       set_immediate_dominator (CDI_DOMINATORS, l1_bb,
 			       recompute_dominator (CDI_DOMINATORS, l1_bb));
 
-      struct loop *outer_loop = alloc_loop ();
-      outer_loop->header = l0_bb;
-      outer_loop->latch = l2_bb;
-      add_loop (outer_loop, l0_bb->loop_father);
+      /* We enter expand_omp_for_generic with a loop.  This original loop may
+	 have its own loop struct, or it may be part of an outer loop struct
+	 (which may be the fake loop).  */
+      struct loop *outer_loop = entry_bb->loop_father;
+      bool orig_loop_has_loop_struct = l1_bb->loop_father != outer_loop;
 
-      if (!gimple_omp_for_combined_p (fd->for_stmt))
+      add_bb_to_loop (l2_bb, outer_loop);
+
+      /* We've added a new loop around the original loop.  Allocate the
+	 corresponding loop struct.  */
+      struct loop *new_loop = alloc_loop ();
+      new_loop->header = l0_bb;
+      new_loop->latch = l2_bb;
+      add_loop (new_loop, outer_loop);
+
+      if (/* If we already have a loop struct for the original loop, don't
+	     allocate a new one.  */
+	  !orig_loop_has_loop_struct
+	  && !gimple_omp_for_combined_p (fd->for_stmt))
 	{
-	  struct loop *loop = alloc_loop ();
-	  loop->header = l1_bb;
+	  struct loop *orig_loop = alloc_loop ();
+	  orig_loop->header = l1_bb;
 	  /* The loop may have multiple latches.  */
-	  add_loop (loop, outer_loop);
+	  add_loop (orig_loop, new_loop);
 	}
     }
 }
-- 
1.9.1