Patchwork tree if convert pass control

login
register
mail settings
Submitter Xinliang David Li
Date Sept. 14, 2013, 6:10 a.m.
Message ID <CAAkRFZL=gmygiRtf7YZWyZdi8-FOrVZJQfyC_=RpuK3W=ZxMtA@mail.gmail.com>
Download mbox | patch
Permalink /patch/274896/
State New
Headers show

Comments

Xinliang David Li - Sept. 14, 2013, 6:10 a.m.
tree if conversion is an enabler pass for vectorization, so by
default, it is only turned on when vectorization is on, but may also
depend on the optimization level. Currently, the logic to handle this
is in the gate function which become hard to understand and extend.

The proposed patch move the logic from the gate function to
'finish_option' which is much clearer. The downside of this patch is
that function specific optimization node needs to be created for some
cases during omp-lowering.

Comments?


thanks,

David
Richard Guenther - Sept. 16, 2013, 9:04 a.m.
On Sat, Sep 14, 2013 at 8:10 AM, Xinliang David Li <davidxl@google.com> wrote:
> tree if conversion is an enabler pass for vectorization, so by
> default, it is only turned on when vectorization is on, but may also
> depend on the optimization level. Currently, the logic to handle this
> is in the gate function which become hard to understand and extend.
>
> The proposed patch move the logic from the gate function to
> 'finish_option' which is much clearer. The downside of this patch is
> that function specific optimization node needs to be created for some
> cases during omp-lowering.

Something I don't like.  What's the issue with checking the
has_force_vect_loops flag?

How's the argument that the gate is hard to extend?  Wouldn't
extending it complicate it again and thus make it hard to understand again?

That said, given that doing things in finish_options () is discouraged
the patch looks like a step backwards.

So, can you explain the underlying rationale?

Btw, if we think of if-conversion as tied to loop vectorization then we can
guard it by it and make a new container pass like

          NEXT_PASS (pass_loop_vectorizer);
          PUSH_INSERT_PASSES_WITHIN (pass_loop_vectorizer)
              NEXT_PASS (pass_if_conversion);
              NEXT_PASS (pass_vectorize);
              NEXT_PASS (pass_dce_loop);
          POP_INSERT_PASSES ()

and guard pass_loop_vectorizer by flag_tree_loop_vectorize, defaulting
if-conversion to be enabled (but allow disabling it manually).

Or take the step and move it under control of the vectorizer itself.

Richard.

> Comments?
>
>
> thanks,
>
> David
Xinliang David Li - Sept. 16, 2013, 9:07 p.m.
Ok -- abandon the patch.

On Mon, Sep 16, 2013 at 2:04 AM, Richard Biener
<richard.guenther@gmail.com> wrote:
> On Sat, Sep 14, 2013 at 8:10 AM, Xinliang David Li <davidxl@google.com> wrote:
>> tree if conversion is an enabler pass for vectorization, so by
>> default, it is only turned on when vectorization is on, but may also
>> depend on the optimization level. Currently, the logic to handle this
>> is in the gate function which become hard to understand and extend.
>>
>> The proposed patch move the logic from the gate function to
>> 'finish_option' which is much clearer. The downside of this patch is
>> that function specific optimization node needs to be created for some
>> cases during omp-lowering.
>
> Something I don't like.  What's the issue with checking the
> has_force_vect_loops flag?
>
> How's the argument that the gate is hard to extend?  Wouldn't
> extending it complicate it again and thus make it hard to understand again?

The gating logic is:

1) the pass is a standalone pass that can be turned on independently
of loop vectorizer;
2) the pass can be implicitly turned on by loop vectorizer, and
possibly depending on the optimization level.

The logic is not complicated, but it would be simplier to handle in
the option processing if there was no need for the 'force_vect_loop'
business.

>
> That said, given that doing things in finish_options () is discouraged
> the patch looks like a step backwards.

Doing it in finish_options can avoid duplicating the handling in
multiple places.

>
> So, can you explain the underlying rationale?
>
> Btw, if we think of if-conversion as tied to loop vectorization then we can
> guard it by it and make a new container pass like
>
>           NEXT_PASS (pass_loop_vectorizer);
>           PUSH_INSERT_PASSES_WITHIN (pass_loop_vectorizer)
>               NEXT_PASS (pass_if_conversion);
>               NEXT_PASS (pass_vectorize);
>               NEXT_PASS (pass_dce_loop);
>           POP_INSERT_PASSES ()
>
> and guard pass_loop_vectorizer by flag_tree_loop_vectorize, defaulting
> if-conversion to be enabled (but allow disabling it manually).
>

This will make it hard to turn on the pass independently.

Anyway, there seems no need for the patch.

thanks,

David


> Or take the step and move it under control of the vectorizer itself.
>
> Richard.
>
>> Comments?
>>
>>
>> thanks,
>>
>> David

Patch

Index: opts.c
===================================================================
--- opts.c	(revision 202229)
+++ opts.c	(working copy)
@@ -824,6 +824,12 @@  finish_options (struct gcc_options *opts
 	}
     }
 
+  if (!opts_set->x_flag_tree_loop_if_convert)
+    opts->x_flag_tree_loop_if_convert = opts->x_flag_tree_vectorize;
+
+  if (!opts_set->x_flag_tree_loop_if_convert_stores)
+    opts->x_flag_tree_loop_if_convert_stores = opts->x_flag_tree_vectorize;
+
   /* Set PARAM_MAX_STORES_TO_SINK to 0 if either vectorization or if-conversion
      is disabled.  */
   if (!opts->x_flag_tree_vectorize || !opts->x_flag_tree_loop_if_convert)
Index: omp-low.c
===================================================================
--- omp-low.c	(revision 202229)
+++ omp-low.c	(working copy)
@@ -5690,8 +5690,36 @@  expand_omp_simd (struct omp_region *regi
 	   || !global_options_set.x_flag_tree_vectorize)
 	  && loop->safelen > 1)
 	{
+          bool need_to_turn_on_if_cvt = false;
+          bool need_to_turn_on_if_cvt_stores = false;
+
 	  loop->force_vect = true;
 	  cfun->has_force_vect_loops = true;
+
+          /* Now check if it is needed to turn on tree-if-convert.  */
+
+          if (!global_options_set.x_flag_tree_loop_if_convert
+              && !flag_tree_loop_if_convert)
+            need_to_turn_on_if_cvt = true;
+
+          if (!global_options_set.x_flag_tree_loop_if_convert_stores
+              && !flag_tree_loop_if_convert_stores)
+            need_to_turn_on_if_cvt_stores = true;
+
+          if (need_to_turn_on_if_cvt || need_to_turn_on_if_cvt_stores)
+            {
+              if (need_to_turn_on_if_cvt)
+                global_options.x_flag_tree_loop_if_convert = 1;
+              if (need_to_turn_on_if_cvt_stores)
+                global_options.x_flag_tree_loop_if_convert_stores = 1;
+              DECL_FUNCTION_SPECIFIC_OPTIMIZATION (current_function_decl)
+                  = build_optimization_node ();
+              /* restore  */
+              if (need_to_turn_on_if_cvt)
+                global_options.x_flag_tree_loop_if_convert = 0;
+              if (need_to_turn_on_if_cvt_stores)
+                global_options.x_flag_tree_loop_if_convert_stores = 0;
+            }
 	}
     }
 }
Index: ChangeLog
===================================================================
--- ChangeLog	(revision 202229)
+++ ChangeLog	(working copy)
@@ -1,3 +1,12 @@ 
+2013-09-13  Xinliang David Li  <davidxl@google.com>
+	
+	* omp-low.c (expand_omp_simd): Build optimization cl
+	node if necessary.
+	* opts.c (finish_option): Enable/Disable tree if-cvt
+	if not explicitly set.
+	* tree-if-conv.c (gate_tree_if_conversion): Simplify.
+
+
 2013-08-29  Xinliang David Li  <davidxl@google.com>
 
 	* tree-vect-slp.c (destroy_bb_vec_info): Data ref cleanup.
Index: common.opt
===================================================================
--- common.opt	(revision 202229)
+++ common.opt	(working copy)
@@ -1308,7 +1308,7 @@  EnumValue
 Enum(stack_reuse_level) String(none) Value(SR_NONE)
 
 ftree-loop-if-convert
-Common Report Var(flag_tree_loop_if_convert) Init(-1) Optimization
+Common Report Var(flag_tree_loop_if_convert) Optimization
 Convert conditional jumps in innermost loops to branchless equivalents
 
 ftree-loop-if-convert-stores
Index: tree-if-conv.c
===================================================================
--- tree-if-conv.c	(revision 202229)
+++ tree-if-conv.c	(working copy)
@@ -1815,10 +1815,8 @@  main_tree_if_conversion (void)
 static bool
 gate_tree_if_conversion (void)
 {
-  return (((flag_tree_vectorize || cfun->has_force_vect_loops)
-	   && flag_tree_loop_if_convert != 0)
-	  || flag_tree_loop_if_convert == 1
-	  || flag_tree_loop_if_convert_stores == 1);
+  return (flag_tree_loop_if_convert
+	  || flag_tree_loop_if_convert_stores);
 }
 
 namespace {