Re: [PATCH] autopar TLC

[Richard Biener <rguenther@suse.de>]

On Tue, 2 Aug 2022, Richard Biener wrote:

> The following removes all excessive update_ssa calls from OMP
> expansion, thereby rewriting the atomic load and store cases to
> GIMPLE code generation.  I don't think autopar ever exercises the
> atomics code though.
> 
> There's not much test coverage overall so I've built SPEC 2k17
> with -floop-parallelize-all -ftree-parallelize-loops=2 with and
> without LTO (and otherwise -Ofast plus -march=haswell) without
> fallout.
> 
> If there's any fallout it's not OK to update SSA form for
> each and every OMP stmt lowered.
> 
> Bootstrapped and tested on x86_64-unknown-linux-gnu.
> 
> Any objections?

I have pushed this now (and will deal with eventual fallout).

Richard.

> Thanks,
> Richard.
> 
> 	* omp-expand.cc (expand_omp_atomic_load): Emit GIMPLE
> 	directly.  Avoid update_ssa when in SSA form.
> 	(expand_omp_atomic_store): Likewise.
> 	(expand_omp_atomic_fetch_op): Avoid update_ssa when in SSA
> 	form.
> 	(expand_omp_atomic_pipeline): Likewise.
> 	(expand_omp_atomic_mutex): Likewise.
> 	* tree-parloops.cc (gen_parallel_loop): Use
> 	TODO_update_ssa_no_phi after loop_version.
> ---
>  gcc/omp-expand.cc    | 81 +++++++++++++++++++++++++++-----------------
>  gcc/tree-parloops.cc |  2 +-
>  2 files changed, 50 insertions(+), 33 deletions(-)
> 
> diff --git a/gcc/omp-expand.cc b/gcc/omp-expand.cc
> index 64e6308fc7b..48fbd157c6e 100644
> --- a/gcc/omp-expand.cc
> +++ b/gcc/omp-expand.cc
> @@ -8617,7 +8617,7 @@ expand_omp_atomic_load (basic_block load_bb, tree addr,
>    basic_block store_bb;
>    location_t loc;
>    gimple *stmt;
> -  tree decl, call, type, itype;
> +  tree decl, type, itype;
>  
>    gsi = gsi_last_nondebug_bb (load_bb);
>    stmt = gsi_stmt (gsi);
> @@ -8637,23 +8637,33 @@ expand_omp_atomic_load (basic_block load_bb, tree addr,
>    itype = TREE_TYPE (TREE_TYPE (decl));
>  
>    enum omp_memory_order omo = gimple_omp_atomic_memory_order (stmt);
> -  tree mo = build_int_cst (NULL, omp_memory_order_to_memmodel (omo));
> -  call = build_call_expr_loc (loc, decl, 2, addr, mo);
> +  tree mo = build_int_cst (integer_type_node,
> +			   omp_memory_order_to_memmodel (omo));
> +  gcall *call = gimple_build_call (decl, 2, addr, mo);
> +  gimple_set_location (call, loc);
> +  gimple_set_vuse (call, gimple_vuse (stmt));
> +  gimple *repl;
>    if (!useless_type_conversion_p (type, itype))
> -    call = fold_build1_loc (loc, VIEW_CONVERT_EXPR, type, call);
> -  call = build2_loc (loc, MODIFY_EXPR, void_type_node, loaded_val, call);
> -
> -  force_gimple_operand_gsi (&gsi, call, true, NULL_TREE, true, GSI_SAME_STMT);
> -  gsi_remove (&gsi, true);
> +    {
> +      tree lhs = make_ssa_name (itype);
> +      gimple_call_set_lhs (call, lhs);
> +      gsi_insert_before (&gsi, call, GSI_SAME_STMT);
> +      repl = gimple_build_assign (loaded_val,
> +				  build1 (VIEW_CONVERT_EXPR, type, lhs));
> +      gimple_set_location (repl, loc);
> +    }
> +  else
> +    {
> +      gimple_call_set_lhs (call, loaded_val);
> +      repl = call;
> +    }
> +  gsi_replace (&gsi, repl, true);
>  
>    store_bb = single_succ (load_bb);
>    gsi = gsi_last_nondebug_bb (store_bb);
>    gcc_assert (gimple_code (gsi_stmt (gsi)) == GIMPLE_OMP_ATOMIC_STORE);
>    gsi_remove (&gsi, true);
>  
> -  if (gimple_in_ssa_p (cfun))
> -    update_ssa (TODO_update_ssa_no_phi);
> -
>    return true;
>  }
>  
> @@ -8669,7 +8679,7 @@ expand_omp_atomic_store (basic_block load_bb, tree addr,
>    basic_block store_bb = single_succ (load_bb);
>    location_t loc;
>    gimple *stmt;
> -  tree decl, call, type, itype;
> +  tree decl, type, itype;
>    machine_mode imode;
>    bool exchange;
>  
> @@ -8710,25 +8720,36 @@ expand_omp_atomic_store (basic_block load_bb, tree addr,
>    if (!useless_type_conversion_p (itype, type))
>      stored_val = fold_build1_loc (loc, VIEW_CONVERT_EXPR, itype, stored_val);
>    enum omp_memory_order omo = gimple_omp_atomic_memory_order (stmt);
> -  tree mo = build_int_cst (NULL, omp_memory_order_to_memmodel (omo));
> -  call = build_call_expr_loc (loc, decl, 3, addr, stored_val, mo);
> +  tree mo = build_int_cst (integer_type_node,
> +			   omp_memory_order_to_memmodel (omo));
> +  stored_val = force_gimple_operand_gsi (&gsi, stored_val, true, NULL_TREE,
> +					 true, GSI_SAME_STMT);
> +  gcall *call = gimple_build_call (decl, 3, addr, stored_val, mo);
> +  gimple_set_location (call, loc);
> +  gimple_set_vuse (call, gimple_vuse (stmt));
> +  gimple_set_vdef (call, gimple_vdef (stmt));
> +
> +  gimple *repl = call;
>    if (exchange)
>      {
>        if (!useless_type_conversion_p (type, itype))
> -	call = build1_loc (loc, VIEW_CONVERT_EXPR, type, call);
> -      call = build2_loc (loc, MODIFY_EXPR, void_type_node, loaded_val, call);
> +	{
> +	  tree lhs = make_ssa_name (itype);
> +	  gimple_call_set_lhs (call, lhs);
> +	  gsi_insert_before (&gsi, call, GSI_SAME_STMT);
> +	  repl = gimple_build_assign (loaded_val,
> +				      build1 (VIEW_CONVERT_EXPR, type, lhs));
> +	  gimple_set_location  (repl, loc);
> +	}
> +      else
> +	gimple_call_set_lhs (call, loaded_val);
>      }
> -
> -  force_gimple_operand_gsi (&gsi, call, true, NULL_TREE, true, GSI_SAME_STMT);
> -  gsi_remove (&gsi, true);
> +  gsi_replace (&gsi, repl, true);
>  
>    /* Remove the GIMPLE_OMP_ATOMIC_LOAD that we verified above.  */
>    gsi = gsi_last_nondebug_bb (load_bb);
>    gsi_remove (&gsi, true);
>  
> -  if (gimple_in_ssa_p (cfun))
> -    update_ssa (TODO_update_ssa_no_phi);
> -
>    return true;
>  }
>  
> @@ -8874,10 +8895,7 @@ expand_omp_atomic_fetch_op (basic_block load_bb,
>    gsi_remove (&gsi, true);
>  
>    if (gimple_in_ssa_p (cfun))
> -    {
> -      release_defs (stmt);
> -      update_ssa (TODO_update_ssa_no_phi);
> -    }
> +    release_defs (stmt);
>  
>    return true;
>  }
> @@ -9333,16 +9351,16 @@ expand_omp_atomic_pipeline (basic_block load_bb, basic_block store_bb,
>      }
>  
>    /* Remove GIMPLE_OMP_ATOMIC_STORE.  */
> +  stmt = gsi_stmt (si);
>    gsi_remove (&si, true);
> +  if (gimple_in_ssa_p (cfun))
> +    release_defs (stmt);
>  
>    class loop *loop = alloc_loop ();
>    loop->header = loop_header;
>    loop->latch = store_bb;
>    add_loop (loop, loop_header->loop_father);
>  
> -  if (gimple_in_ssa_p (cfun))
> -    update_ssa (TODO_update_ssa_no_phi);
> -
>    return true;
>  }
>  
> @@ -9399,15 +9417,14 @@ expand_omp_atomic_mutex (basic_block load_bb, basic_block store_bb,
>    gcc_assert (gimple_code (gsi_stmt (si)) == GIMPLE_OMP_ATOMIC_STORE);
>  
>    stmt = gimple_build_assign (unshare_expr (mem), stored_val);
> +  gimple_set_vuse (stmt, gimple_vuse (gsi_stmt (si)));
> +  gimple_set_vdef (stmt, gimple_vdef (gsi_stmt (si)));
>    gsi_insert_before (&si, stmt, GSI_SAME_STMT);
>  
>    t = builtin_decl_explicit (BUILT_IN_GOMP_ATOMIC_END);
>    t = build_call_expr (t, 0);
>    force_gimple_operand_gsi (&si, t, true, NULL_TREE, true, GSI_SAME_STMT);
>    gsi_remove (&si, true);
> -
> -  if (gimple_in_ssa_p (cfun))
> -    update_ssa (TODO_update_ssa_no_phi);
>    return true;
>  }
>  
> diff --git a/gcc/tree-parloops.cc b/gcc/tree-parloops.cc
> index 2d3aa78cd24..b070527ee6e 100644
> --- a/gcc/tree-parloops.cc
> +++ b/gcc/tree-parloops.cc
> @@ -3082,7 +3082,7 @@ gen_parallel_loop (class loop *loop,
>  		    profile_probability::unlikely (),
>  		    profile_probability::likely (),
>  		    profile_probability::unlikely (), true);
> -      update_ssa (TODO_update_ssa);
> +      update_ssa (TODO_update_ssa_no_phi);
>        free_original_copy_tables ();
>      }
>  
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Frankenstrasse 146, 90461 Nuernberg,
Germany; GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman;
HRB 36809 (AG Nuernberg)