Re: [PATCH] Fix __builtin_powi placement (PR52976 follow-up)

[Richard Guenther <rguenther at suse dot de>]

On Tue, 17 Apr 2012, William J. Schmidt wrote:

> The emergency patch for PR52976 manipulated the operand rank system to
> force inserted __builtin_powi calls to occur before uses of the call
> results.  However, this is generally the wrong approach, as it forces
> other computations to move unnecessarily, and extends the lifetimes of
> other operands.
> 
> This patch fixes the problem in the proper way, by letting the rank
> system determine where the __builtin_powi call belongs, and moving the
> call to that location during the expression rewrite.
> 
> Bootstrapped with no new regressions on powerpc64-linux.  SPEC cpu2000
> and cpu2006 also build cleanly.  Ok for trunk?

Ok.

Thanks,
Richard.

> Thanks,
> Bill
> 
> 
> gcc:
> 
> 2012-04-17  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>
> 
> 	* tree-ssa-reassoc.c (add_to_ops_vec_max_rank): Delete.
> 	(possibly_move_powi): New function.
> 	(rewrite_expr_tree): Call possibly_move_powi.
> 	(rewrite_expr_tree_parallel): Likewise.
> 	(attempt_builtin_powi): Change call of add_to_ops_vec_max_rank to
> 	call add_to_ops_vec instead.
> 
> 
> gcc/testsuite:
> 
> 2012-04-17  Bill Schmidt  <wschmidt@linux.vnet.ibm.com>
> 
> 	gfortran.dg/reassoc_11.f: New test.
> 
> 
> 
> Index: gcc/testsuite/gfortran.dg/reassoc_11.f
> ===================================================================
> --- gcc/testsuite/gfortran.dg/reassoc_11.f	(revision 0)
> +++ gcc/testsuite/gfortran.dg/reassoc_11.f	(revision 0)
> @@ -0,0 +1,17 @@
> +! { dg-do compile }
> +! { dg-options "-O3 -ffast-math" }
> +
> +! This tests only for compile-time failure, which formerly occurred
> +! when a __builtin_powi was introduced by reassociation in a bad place.
> +
> +      SUBROUTINE GRDURBAN(URBWSTR, ZIURB, GRIDHT)
> +
> +      IMPLICIT NONE
> +      INTEGER :: I
> +      REAL :: SW2, URBWSTR, ZIURB, GRIDHT(87)
> +
> +      SAVE 
> +
> +      SW2 = 1.6*(GRIDHT(I)/ZIURB)**0.667*URBWSTR**2
> +
> +      END
> Index: gcc/tree-ssa-reassoc.c
> ===================================================================
> --- gcc/tree-ssa-reassoc.c	(revision 186495)
> +++ gcc/tree-ssa-reassoc.c	(working copy)
> @@ -544,28 +544,6 @@ add_repeat_to_ops_vec (VEC(operand_entry_t, heap)
>    reassociate_stats.pows_encountered++;
>  }
>  
> -/* Add an operand entry to *OPS for the tree operand OP, giving the
> -   new entry a larger rank than any other operand already in *OPS.  */
> -
> -static void
> -add_to_ops_vec_max_rank (VEC(operand_entry_t, heap) **ops, tree op)
> -{
> -  operand_entry_t oe = (operand_entry_t) pool_alloc (operand_entry_pool);
> -  operand_entry_t oe1;
> -  unsigned i;
> -  unsigned max_rank = 0;
> -
> -  FOR_EACH_VEC_ELT (operand_entry_t, *ops, i, oe1)
> -    if (oe1->rank > max_rank)
> -      max_rank = oe1->rank;
> -
> -  oe->op = op;
> -  oe->rank = max_rank + 1;
> -  oe->id = next_operand_entry_id++;
> -  oe->count = 1;
> -  VEC_safe_push (operand_entry_t, heap, *ops, oe);
> -}
> -
>  /* Return true if STMT is reassociable operation containing a binary
>     operation with tree code CODE, and is inside LOOP.  */
>  
> @@ -2162,6 +2242,47 @@ remove_visited_stmt_chain (tree var)
>      }
>  }
>  
> +/* If OP is an SSA name, find its definition and determine whether it
> +   is a call to __builtin_powi.  If so, move the definition prior to
> +   STMT.  Only do this during early reassociation.  */
> +
> +static void
> +possibly_move_powi (gimple stmt, tree op)
> +{
> +  gimple stmt2;
> +  tree fndecl;
> +  gimple_stmt_iterator gsi1, gsi2;
> +
> +  if (!first_pass_instance
> +      || !flag_unsafe_math_optimizations
> +      || TREE_CODE (op) != SSA_NAME)
> +    return;
> +  
> +  stmt2 = SSA_NAME_DEF_STMT (op);
> +
> +  if (!is_gimple_call (stmt2)
> +      || !has_single_use (gimple_call_lhs (stmt2)))
> +    return;
> +
> +  fndecl = gimple_call_fndecl (stmt2);
> +
> +  if (!fndecl
> +      || DECL_BUILT_IN_CLASS (fndecl) != BUILT_IN_NORMAL)
> +    return;
> +
> +  switch (DECL_FUNCTION_CODE (fndecl))
> +    {
> +    CASE_FLT_FN (BUILT_IN_POWI):
> +      break;
> +    default:
> +      return;
> +    }
> +
> +  gsi1 = gsi_for_stmt (stmt);
> +  gsi2 = gsi_for_stmt (stmt2);
> +  gsi_move_before (&gsi2, &gsi1);
> +}
> +
>  /* This function checks three consequtive operands in
>     passed operands vector OPS starting from OPINDEX and
>     swaps two operands if it is profitable for binary operation
> @@ -2267,6 +2388,8 @@ rewrite_expr_tree (gimple stmt, unsigned int opind
>  	      print_gimple_stmt (dump_file, stmt, 0, 0);
>  	    }
>  
> +	  possibly_move_powi (stmt, oe1->op);
> +	  possibly_move_powi (stmt, oe2->op);
>  	}
>        return;
>      }
> @@ -2312,6 +2435,8 @@ rewrite_expr_tree (gimple stmt, unsigned int opind
>  	  fprintf (dump_file, " into ");
>  	  print_gimple_stmt (dump_file, stmt, 0, 0);
>  	}
> +
> +      possibly_move_powi (stmt, oe->op);
>      }
>    /* Recurse on the LHS of the binary operator, which is guaranteed to
>       be the non-leaf side.  */
> @@ -2485,6 +2610,9 @@ rewrite_expr_tree_parallel (gimple stmt, int width
>  	  fprintf (dump_file, " into ");
>  	  print_gimple_stmt (dump_file, stmts[i], 0, 0);
>  	}
> +
> +      possibly_move_powi (stmts[i], op1);
> +      possibly_move_powi (stmts[i], op2);
>      }
>  
>    remove_visited_stmt_chain (last_rhs1);
> @@ -3196,6 +3324,8 @@ attempt_builtin_powi (gimple stmt, VEC(operand_ent
>  							   power));
>  	      gimple_call_set_lhs (pow_stmt, iter_result);
>  	      gimple_set_location (pow_stmt, gimple_location (stmt));
> +	      /* Temporarily place the call; we will move it to the
> +		 correct place during rewrite_expr.  */
>  	      gsi_insert_before (&gsi, pow_stmt, GSI_SAME_STMT);
>  
>  	      if (dump_file && (dump_flags & TDF_DETAILS))
> @@ -3300,10 +3430,8 @@ attempt_builtin_powi (gimple stmt, VEC(operand_ent
>  	  gsi_insert_before (&gsi, pow_stmt, GSI_SAME_STMT);
>  	}
>  
> -      /* Append the result of this iteration to the ops vector.
> -         Give it a rank higher than all other ranks in the ops vector
> -         so that all uses of it will be forced to come after it.  */
> -      add_to_ops_vec_max_rank (ops, iter_result);
> +      /* Append the result of this iteration to the ops vector.  */
> +      add_to_ops_vec (ops, iter_result);
>  
>        /* Decrement the occurrence count of each element in the product
>  	 by the count found above, and remove this many copies of each
> 
> 
> 

-- 
Richard Guenther <rguenther@suse.de>
SUSE / SUSE Labs
SUSE LINUX Products GmbH - Nuernberg - AG Nuernberg - HRB 16746
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer