Patchwork RFA: Fix reuse of "target" variable in builtins.c

login
register
mail settings
Submitter Richard Sandiford
Date Dec. 23, 2012, 5:02 p.m.
Message ID <878v8ovh0v.fsf@talisman.default>
Download mbox | patch
Permalink /patch/207989/
State New
Headers show

Comments

Richard Sandiford - Dec. 23, 2012, 5:02 p.m.
Some of the maths builtins try to expand via an optab and fall back
to an expand_call.  The optabs path tends to clobber "target",
so the original target is lost by the time we call expand_call.  E.g.:

  /* Compute into TARGET.
     Set TARGET to wherever the result comes back.  */
  target = expand_binop (mode, builtin_optab, op0, op1,
			 target, 0, OPTAB_DIRECT);

  /* If we were unable to expand via the builtin, stop the sequence
     (without outputting the insns) and call to the library function
     with the stabilized argument list.  */
  if (target == 0)
    {
      end_sequence ();
      return expand_call (exp, target, target == const0_rtx);
    }

where the expand_call seems to be trying to use the original call target
(as it should IMO) but actually always uses null.

This patch tries to fix the cases I could see.  Tested on x86_64-linux-gnu
and mips64-linux-gnu.  OK to install?

Richard


gcc/
	* builtins.c (expand_builtin_mathfn, expand_builtin_mathfn_2)
	(expand_builtin_mathfn_ternary, expand_builtin_mathfn_3)
	(expand_builtin_int_roundingfn_2): Keep the original target around
	for the fallback case.
Richard Guenther - Dec. 30, 2012, 7:24 p.m.
On Sun, Dec 23, 2012 at 6:02 PM, Richard Sandiford
<rdsandiford@googlemail.com> wrote:
> Some of the maths builtins try to expand via an optab and fall back
> to an expand_call.  The optabs path tends to clobber "target",
> so the original target is lost by the time we call expand_call.  E.g.:
>
>   /* Compute into TARGET.
>      Set TARGET to wherever the result comes back.  */
>   target = expand_binop (mode, builtin_optab, op0, op1,
>                          target, 0, OPTAB_DIRECT);
>
>   /* If we were unable to expand via the builtin, stop the sequence
>      (without outputting the insns) and call to the library function
>      with the stabilized argument list.  */
>   if (target == 0)
>     {
>       end_sequence ();
>       return expand_call (exp, target, target == const0_rtx);
>     }
>
> where the expand_call seems to be trying to use the original call target
> (as it should IMO) but actually always uses null.
>
> This patch tries to fix the cases I could see.  Tested on x86_64-linux-gnu
> and mips64-linux-gnu.  OK to install?

Ok.

Thanks,
Richard.

> Richard
>
>
> gcc/
>         * builtins.c (expand_builtin_mathfn, expand_builtin_mathfn_2)
>         (expand_builtin_mathfn_ternary, expand_builtin_mathfn_3)
>         (expand_builtin_int_roundingfn_2): Keep the original target around
>         for the fallback case.
>
> Index: gcc/builtins.c
> ===================================================================
> --- gcc/builtins.c      2012-12-23 16:56:30.218846420 +0000
> +++ gcc/builtins.c      2012-12-23 16:57:47.849415792 +0000
> @@ -2031,7 +2031,7 @@ expand_builtin_mathfn (tree exp, rtx tar
>    if (optab_handler (builtin_optab, mode) != CODE_FOR_nothing
>        && (!errno_set || !optimize_insn_for_size_p ()))
>      {
> -      target = gen_reg_rtx (mode);
> +      rtx result = gen_reg_rtx (mode);
>
>        /* Wrap the computation of the argument in a SAVE_EXPR, as we may
>          need to expand the argument again.  This way, we will not perform
> @@ -2042,20 +2042,20 @@ expand_builtin_mathfn (tree exp, rtx tar
>
>        start_sequence ();
>
> -      /* Compute into TARGET.
> -        Set TARGET to wherever the result comes back.  */
> -      target = expand_unop (mode, builtin_optab, op0, target, 0);
> +      /* Compute into RESULT.
> +        Set RESULT to wherever the result comes back.  */
> +      result = expand_unop (mode, builtin_optab, op0, result, 0);
>
> -      if (target != 0)
> +      if (result != 0)
>         {
>           if (errno_set)
> -           expand_errno_check (exp, target);
> +           expand_errno_check (exp, result);
>
>           /* Output the entire sequence.  */
>           insns = get_insns ();
>           end_sequence ();
>           emit_insn (insns);
> -         return target;
> +         return result;
>         }
>
>        /* If we were unable to expand via the builtin, stop the sequence
> @@ -2078,7 +2078,7 @@ expand_builtin_mathfn (tree exp, rtx tar
>  expand_builtin_mathfn_2 (tree exp, rtx target, rtx subtarget)
>  {
>    optab builtin_optab;
> -  rtx op0, op1, insns;
> +  rtx op0, op1, insns, result;
>    int op1_type = REAL_TYPE;
>    tree fndecl = get_callee_fndecl (exp);
>    tree arg0, arg1;
> @@ -2134,7 +2134,7 @@ expand_builtin_mathfn_2 (tree exp, rtx t
>    if (optab_handler (builtin_optab, mode) == CODE_FOR_nothing)
>      return NULL_RTX;
>
> -  target = gen_reg_rtx (mode);
> +  result = gen_reg_rtx (mode);
>
>    if (! flag_errno_math || ! HONOR_NANS (mode))
>      errno_set = false;
> @@ -2151,29 +2151,29 @@ expand_builtin_mathfn_2 (tree exp, rtx t
>
>    start_sequence ();
>
> -  /* Compute into TARGET.
> -     Set TARGET to wherever the result comes back.  */
> -  target = expand_binop (mode, builtin_optab, op0, op1,
> -                        target, 0, OPTAB_DIRECT);
> +  /* Compute into RESULT.
> +     Set RESULT to wherever the result comes back.  */
> +  result = expand_binop (mode, builtin_optab, op0, op1,
> +                        result, 0, OPTAB_DIRECT);
>
>    /* If we were unable to expand via the builtin, stop the sequence
>       (without outputting the insns) and call to the library function
>       with the stabilized argument list.  */
> -  if (target == 0)
> +  if (result == 0)
>      {
>        end_sequence ();
>        return expand_call (exp, target, target == const0_rtx);
>      }
>
>    if (errno_set)
> -    expand_errno_check (exp, target);
> +    expand_errno_check (exp, result);
>
>    /* Output the entire sequence.  */
>    insns = get_insns ();
>    end_sequence ();
>    emit_insn (insns);
>
> -  return target;
> +  return result;
>  }
>
>  /* Expand a call to the builtin trinary math functions (fma).
> @@ -2187,7 +2187,7 @@ expand_builtin_mathfn_2 (tree exp, rtx t
>  expand_builtin_mathfn_ternary (tree exp, rtx target, rtx subtarget)
>  {
>    optab builtin_optab;
> -  rtx op0, op1, op2, insns;
> +  rtx op0, op1, op2, insns, result;
>    tree fndecl = get_callee_fndecl (exp);
>    tree arg0, arg1, arg2;
>    enum machine_mode mode;
> @@ -2214,7 +2214,7 @@ expand_builtin_mathfn_ternary (tree exp,
>    if (optab_handler (builtin_optab, mode) == CODE_FOR_nothing)
>      return NULL_RTX;
>
> -  target = gen_reg_rtx (mode);
> +  result = gen_reg_rtx (mode);
>
>    /* Always stabilize the argument list.  */
>    CALL_EXPR_ARG (exp, 0) = arg0 = builtin_save_expr (arg0);
> @@ -2227,15 +2227,15 @@ expand_builtin_mathfn_ternary (tree exp,
>
>    start_sequence ();
>
> -  /* Compute into TARGET.
> -     Set TARGET to wherever the result comes back.  */
> -  target = expand_ternary_op (mode, builtin_optab, op0, op1, op2,
> -                             target, 0);
> +  /* Compute into RESULT.
> +     Set RESULT to wherever the result comes back.  */
> +  result = expand_ternary_op (mode, builtin_optab, op0, op1, op2,
> +                             result, 0);
>
>    /* If we were unable to expand via the builtin, stop the sequence
>       (without outputting the insns) and call to the library function
>       with the stabilized argument list.  */
> -  if (target == 0)
> +  if (result == 0)
>      {
>        end_sequence ();
>        return expand_call (exp, target, target == const0_rtx);
> @@ -2246,7 +2246,7 @@ expand_builtin_mathfn_ternary (tree exp,
>    end_sequence ();
>    emit_insn (insns);
>
> -  return target;
> +  return result;
>  }
>
>  /* Expand a call to the builtin sin and cos math functions.
> @@ -2298,7 +2298,7 @@ expand_builtin_mathfn_3 (tree exp, rtx t
>    /* Before working hard, check whether the instruction is available.  */
>    if (optab_handler (builtin_optab, mode) != CODE_FOR_nothing)
>      {
> -      target = gen_reg_rtx (mode);
> +      rtx result = gen_reg_rtx (mode);
>
>        /* Wrap the computation of the argument in a SAVE_EXPR, as we may
>          need to expand the argument again.  This way, we will not perform
> @@ -2309,37 +2309,35 @@ expand_builtin_mathfn_3 (tree exp, rtx t
>
>        start_sequence ();
>
> -      /* Compute into TARGET.
> -        Set TARGET to wherever the result comes back.  */
> +      /* Compute into RESULT.
> +        Set RESULT to wherever the result comes back.  */
>        if (builtin_optab == sincos_optab)
>         {
> -         int result;
> +         int ok;
>
>           switch (DECL_FUNCTION_CODE (fndecl))
>             {
>             CASE_FLT_FN (BUILT_IN_SIN):
> -             result = expand_twoval_unop (builtin_optab, op0, 0, target, 0);
> +             ok = expand_twoval_unop (builtin_optab, op0, 0, result, 0);
>               break;
>             CASE_FLT_FN (BUILT_IN_COS):
> -             result = expand_twoval_unop (builtin_optab, op0, target, 0, 0);
> +             ok = expand_twoval_unop (builtin_optab, op0, result, 0, 0);
>               break;
>             default:
>               gcc_unreachable ();
>             }
> -         gcc_assert (result);
> +         gcc_assert (ok);
>         }
>        else
> -       {
> -         target = expand_unop (mode, builtin_optab, op0, target, 0);
> -       }
> +       result = expand_unop (mode, builtin_optab, op0, result, 0);
>
> -      if (target != 0)
> +      if (result != 0)
>         {
>           /* Output the entire sequence.  */
>           insns = get_insns ();
>           end_sequence ();
>           emit_insn (insns);
> -         return target;
> +         return result;
>         }
>
>        /* If we were unable to expand via the builtin, stop the sequence
> @@ -2348,9 +2346,7 @@ expand_builtin_mathfn_3 (tree exp, rtx t
>        end_sequence ();
>      }
>
> -  target = expand_call (exp, target, target == const0_rtx);
> -
> -  return target;
> +  return expand_call (exp, target, target == const0_rtx);
>  }
>
>  /* Given an interclass math builtin decl FNDECL and it's argument ARG
> @@ -2820,7 +2816,7 @@ expand_builtin_int_roundingfn_2 (tree ex
>    /* There's no easy way to detect the case we need to set EDOM.  */
>    if (!flag_errno_math)
>      {
> -      target = gen_reg_rtx (mode);
> +      rtx result = gen_reg_rtx (mode);
>
>        /* Wrap the computation of the argument in a SAVE_EXPR, as we may
>          need to expand the argument again.  This way, we will not perform
> @@ -2831,13 +2827,13 @@ expand_builtin_int_roundingfn_2 (tree ex
>
>        start_sequence ();
>
> -      if (expand_sfix_optab (target, op0, builtin_optab))
> +      if (expand_sfix_optab (result, op0, builtin_optab))
>         {
>           /* Output the entire sequence.  */
>           insns = get_insns ();
>           end_sequence ();
>           emit_insn (insns);
> -         return target;
> +         return result;
>         }
>
>        /* If we were unable to expand via the builtin, stop the sequence
> @@ -2865,9 +2861,7 @@ expand_builtin_int_roundingfn_2 (tree ex
>        return convert_to_mode (mode, target, 0);
>      }
>
> -  target = expand_call (exp, target, target == const0_rtx);
> -
> -  return target;
> +  return expand_call (exp, target, target == const0_rtx);
>  }
>
>  /* Expand a call to the powi built-in mathematical function.  Return NULL_RTX if

Patch

Index: gcc/builtins.c
===================================================================
--- gcc/builtins.c	2012-12-23 16:56:30.218846420 +0000
+++ gcc/builtins.c	2012-12-23 16:57:47.849415792 +0000
@@ -2031,7 +2031,7 @@  expand_builtin_mathfn (tree exp, rtx tar
   if (optab_handler (builtin_optab, mode) != CODE_FOR_nothing
       && (!errno_set || !optimize_insn_for_size_p ()))
     {
-      target = gen_reg_rtx (mode);
+      rtx result = gen_reg_rtx (mode);
 
       /* Wrap the computation of the argument in a SAVE_EXPR, as we may
 	 need to expand the argument again.  This way, we will not perform
@@ -2042,20 +2042,20 @@  expand_builtin_mathfn (tree exp, rtx tar
 
       start_sequence ();
 
-      /* Compute into TARGET.
-	 Set TARGET to wherever the result comes back.  */
-      target = expand_unop (mode, builtin_optab, op0, target, 0);
+      /* Compute into RESULT.
+	 Set RESULT to wherever the result comes back.  */
+      result = expand_unop (mode, builtin_optab, op0, result, 0);
 
-      if (target != 0)
+      if (result != 0)
 	{
 	  if (errno_set)
-	    expand_errno_check (exp, target);
+	    expand_errno_check (exp, result);
 
 	  /* Output the entire sequence.  */
 	  insns = get_insns ();
 	  end_sequence ();
 	  emit_insn (insns);
-	  return target;
+	  return result;
 	}
 
       /* If we were unable to expand via the builtin, stop the sequence
@@ -2078,7 +2078,7 @@  expand_builtin_mathfn (tree exp, rtx tar
 expand_builtin_mathfn_2 (tree exp, rtx target, rtx subtarget)
 {
   optab builtin_optab;
-  rtx op0, op1, insns;
+  rtx op0, op1, insns, result;
   int op1_type = REAL_TYPE;
   tree fndecl = get_callee_fndecl (exp);
   tree arg0, arg1;
@@ -2134,7 +2134,7 @@  expand_builtin_mathfn_2 (tree exp, rtx t
   if (optab_handler (builtin_optab, mode) == CODE_FOR_nothing)
     return NULL_RTX;
 
-  target = gen_reg_rtx (mode);
+  result = gen_reg_rtx (mode);
 
   if (! flag_errno_math || ! HONOR_NANS (mode))
     errno_set = false;
@@ -2151,29 +2151,29 @@  expand_builtin_mathfn_2 (tree exp, rtx t
 
   start_sequence ();
 
-  /* Compute into TARGET.
-     Set TARGET to wherever the result comes back.  */
-  target = expand_binop (mode, builtin_optab, op0, op1,
-			 target, 0, OPTAB_DIRECT);
+  /* Compute into RESULT.
+     Set RESULT to wherever the result comes back.  */
+  result = expand_binop (mode, builtin_optab, op0, op1,
+			 result, 0, OPTAB_DIRECT);
 
   /* If we were unable to expand via the builtin, stop the sequence
      (without outputting the insns) and call to the library function
      with the stabilized argument list.  */
-  if (target == 0)
+  if (result == 0)
     {
       end_sequence ();
       return expand_call (exp, target, target == const0_rtx);
     }
 
   if (errno_set)
-    expand_errno_check (exp, target);
+    expand_errno_check (exp, result);
 
   /* Output the entire sequence.  */
   insns = get_insns ();
   end_sequence ();
   emit_insn (insns);
 
-  return target;
+  return result;
 }
 
 /* Expand a call to the builtin trinary math functions (fma).
@@ -2187,7 +2187,7 @@  expand_builtin_mathfn_2 (tree exp, rtx t
 expand_builtin_mathfn_ternary (tree exp, rtx target, rtx subtarget)
 {
   optab builtin_optab;
-  rtx op0, op1, op2, insns;
+  rtx op0, op1, op2, insns, result;
   tree fndecl = get_callee_fndecl (exp);
   tree arg0, arg1, arg2;
   enum machine_mode mode;
@@ -2214,7 +2214,7 @@  expand_builtin_mathfn_ternary (tree exp,
   if (optab_handler (builtin_optab, mode) == CODE_FOR_nothing)
     return NULL_RTX;
 
-  target = gen_reg_rtx (mode);
+  result = gen_reg_rtx (mode);
 
   /* Always stabilize the argument list.  */
   CALL_EXPR_ARG (exp, 0) = arg0 = builtin_save_expr (arg0);
@@ -2227,15 +2227,15 @@  expand_builtin_mathfn_ternary (tree exp,
 
   start_sequence ();
 
-  /* Compute into TARGET.
-     Set TARGET to wherever the result comes back.  */
-  target = expand_ternary_op (mode, builtin_optab, op0, op1, op2,
-			      target, 0);
+  /* Compute into RESULT.
+     Set RESULT to wherever the result comes back.  */
+  result = expand_ternary_op (mode, builtin_optab, op0, op1, op2,
+			      result, 0);
 
   /* If we were unable to expand via the builtin, stop the sequence
      (without outputting the insns) and call to the library function
      with the stabilized argument list.  */
-  if (target == 0)
+  if (result == 0)
     {
       end_sequence ();
       return expand_call (exp, target, target == const0_rtx);
@@ -2246,7 +2246,7 @@  expand_builtin_mathfn_ternary (tree exp,
   end_sequence ();
   emit_insn (insns);
 
-  return target;
+  return result;
 }
 
 /* Expand a call to the builtin sin and cos math functions.
@@ -2298,7 +2298,7 @@  expand_builtin_mathfn_3 (tree exp, rtx t
   /* Before working hard, check whether the instruction is available.  */
   if (optab_handler (builtin_optab, mode) != CODE_FOR_nothing)
     {
-      target = gen_reg_rtx (mode);
+      rtx result = gen_reg_rtx (mode);
 
       /* Wrap the computation of the argument in a SAVE_EXPR, as we may
 	 need to expand the argument again.  This way, we will not perform
@@ -2309,37 +2309,35 @@  expand_builtin_mathfn_3 (tree exp, rtx t
 
       start_sequence ();
 
-      /* Compute into TARGET.
-	 Set TARGET to wherever the result comes back.  */
+      /* Compute into RESULT.
+	 Set RESULT to wherever the result comes back.  */
       if (builtin_optab == sincos_optab)
 	{
-	  int result;
+	  int ok;
 
 	  switch (DECL_FUNCTION_CODE (fndecl))
 	    {
 	    CASE_FLT_FN (BUILT_IN_SIN):
-	      result = expand_twoval_unop (builtin_optab, op0, 0, target, 0);
+	      ok = expand_twoval_unop (builtin_optab, op0, 0, result, 0);
 	      break;
 	    CASE_FLT_FN (BUILT_IN_COS):
-	      result = expand_twoval_unop (builtin_optab, op0, target, 0, 0);
+	      ok = expand_twoval_unop (builtin_optab, op0, result, 0, 0);
 	      break;
 	    default:
 	      gcc_unreachable ();
 	    }
-	  gcc_assert (result);
+	  gcc_assert (ok);
 	}
       else
-	{
-	  target = expand_unop (mode, builtin_optab, op0, target, 0);
-	}
+	result = expand_unop (mode, builtin_optab, op0, result, 0);
 
-      if (target != 0)
+      if (result != 0)
 	{
 	  /* Output the entire sequence.  */
 	  insns = get_insns ();
 	  end_sequence ();
 	  emit_insn (insns);
-	  return target;
+	  return result;
 	}
 
       /* If we were unable to expand via the builtin, stop the sequence
@@ -2348,9 +2346,7 @@  expand_builtin_mathfn_3 (tree exp, rtx t
       end_sequence ();
     }
 
-  target = expand_call (exp, target, target == const0_rtx);
-
-  return target;
+  return expand_call (exp, target, target == const0_rtx);
 }
 
 /* Given an interclass math builtin decl FNDECL and it's argument ARG
@@ -2820,7 +2816,7 @@  expand_builtin_int_roundingfn_2 (tree ex
   /* There's no easy way to detect the case we need to set EDOM.  */
   if (!flag_errno_math)
     {
-      target = gen_reg_rtx (mode);
+      rtx result = gen_reg_rtx (mode);
 
       /* Wrap the computation of the argument in a SAVE_EXPR, as we may
 	 need to expand the argument again.  This way, we will not perform
@@ -2831,13 +2827,13 @@  expand_builtin_int_roundingfn_2 (tree ex
 
       start_sequence ();
 
-      if (expand_sfix_optab (target, op0, builtin_optab))
+      if (expand_sfix_optab (result, op0, builtin_optab))
 	{
 	  /* Output the entire sequence.  */
 	  insns = get_insns ();
 	  end_sequence ();
 	  emit_insn (insns);
-	  return target;
+	  return result;
 	}
 
       /* If we were unable to expand via the builtin, stop the sequence
@@ -2865,9 +2861,7 @@  expand_builtin_int_roundingfn_2 (tree ex
       return convert_to_mode (mode, target, 0);
     }
 
-  target = expand_call (exp, target, target == const0_rtx);
-
-  return target;
+  return expand_call (exp, target, target == const0_rtx);
 }
 
 /* Expand a call to the powi built-in mathematical function.  Return NULL_RTX if