Patchwork Tree tail merging breaks __builtin_unreachable optimization

login
register
mail settings
Submitter Tom de Vries
Date July 5, 2012, 6:45 p.m.
Message ID <4FF5E0D1.30402@mentor.com>
Download mbox | patch
Permalink /patch/169236/
State New
Headers show

Comments

Tom de Vries - July 5, 2012, 6:45 p.m.
On 05/07/12 15:30, Michael Matz wrote:
> Hi,
> 
> On Thu, 5 Jul 2012, Tom de Vries wrote:
> 
>> The asserts allow the return result to be optimized, but not the cfg 
>> conditions.
>>
>> AFAIU, we can insert the asserts earlier. F.i., we can insert
>>   aD.1711_6 = ASSERT_EXPR <aD.1711_1(D), aD.1711_1(D) > 0>
>> before the GIMPLE_COND in bb2.
> 
> Nope.  That would require some more checks, in particular that the BB 
> containing builtin_unreachable doesn't contain any other side-effects.  
> Given this:
> 
> if (i < 0)
>   { do_something_interesting();
>     __builtin_unreachable();
>   }
> 
> moving the assert before the if would remove the if condition, hence 
> the call to do_something_interesting.  You need to retain side-effects if 
> there are any.
> 

Michael,

Thanks for pointing that out.

I tried a first stab at your suggestion of implementing the optimization in
pass_fold_builtins, it works for the test-case.

Thanks,
- Tom

> 
> Ciao,
> Michael.
>
Richard Guenther - July 6, 2012, 11:01 a.m.
On Thu, Jul 5, 2012 at 8:45 PM, Tom de Vries <Tom_deVries@mentor.com> wrote:
> On 05/07/12 15:30, Michael Matz wrote:
>> Hi,
>>
>> On Thu, 5 Jul 2012, Tom de Vries wrote:
>>
>>> The asserts allow the return result to be optimized, but not the cfg
>>> conditions.
>>>
>>> AFAIU, we can insert the asserts earlier. F.i., we can insert
>>>   aD.1711_6 = ASSERT_EXPR <aD.1711_1(D), aD.1711_1(D) > 0>
>>> before the GIMPLE_COND in bb2.
>>
>> Nope.  That would require some more checks, in particular that the BB
>> containing builtin_unreachable doesn't contain any other side-effects.
>> Given this:
>>
>> if (i < 0)
>>   { do_something_interesting();
>>     __builtin_unreachable();
>>   }
>>
>> moving the assert before the if would remove the if condition, hence
>> the call to do_something_interesting.  You need to retain side-effects if
>> there are any.
>>
>
> Michael,
>
> Thanks for pointing that out.
>
> I tried a first stab at your suggestion of implementing the optimization in
> pass_fold_builtins, it works for the test-case.

+static bool
+optimize_unreachable (gimple_stmt_iterator i)
+{
+  gimple stmt;
+  basic_block bb;
+  edge_iterator ei;
+  edge e;
+
+  for (gsi_prev (&i); !gsi_end_p (i); gsi_prev (&i))
+    {
+      stmt = gsi_stmt (i);
+      if (gimple_has_side_effects (stmt))
+       return false;
+    }

I think we should rely on DCE to remove stmts without side-effects before
__builtin_unreachable.  Thus I'd make this

  basic_block bb = gsi_bb (i);
  for (gsi = gsi_start (bb); !gsi_end_p (i); gsi_next (&gsi))
    {
      if (is_gimple_debug ())
       continue;
      if (gimple_code () == GIMPLE_LABEL)
        /* Verify we do not need to preserve the label.  */;
      if (gsi_stmt () != gsi_stmt (i))
        return false;
    }

  ...

thus simply require the builtin be the first statement in the block.

As for the label I'm concerned about

void foo (int b, int c)
{
  void *x = &&lab;
  if (b)
    {
lab:
      __builtin_unreachable ();
    }
lab2:
  if (c)
    x = &&lab2;
  goto *x;
}

non-sensical, of course, but "valid".

Richard.

> Thanks,
> - Tom
>
>>
>> Ciao,
>> Michael.
>>
>
>

Patch

Index: gcc/tree-ssa-ccp.c
===================================================================
--- gcc/tree-ssa-ccp.c (revision 189007)
+++ gcc/tree-ssa-ccp.c (working copy)
@@ -2318,6 +2318,44 @@  optimize_stdarg_builtin (gimple call)
     }
 }
 
+/* Return false if there are staments with side-effects before I.  Otherwise,
+   return true and make the block of I unreachable.  */
+
+static bool
+optimize_unreachable (gimple_stmt_iterator i)
+{
+  gimple stmt;
+  basic_block bb;
+  edge_iterator ei;
+  edge e;
+
+  for (gsi_prev (&i); !gsi_end_p (i); gsi_prev (&i))
+    {
+      stmt = gsi_stmt (i);
+      if (gimple_has_side_effects (stmt))
+	return false;
+    }
+
+  bb = gsi_bb (i);
+
+  FOR_EACH_EDGE (e, ei, bb->preds)
+    {
+      basic_block src = e->src;
+      gimple stmt;
+      i = gsi_last_bb (src);
+      stmt = gsi_stmt (i);
+      gcc_assert (gimple_code (stmt) == GIMPLE_COND);
+      if (e->flags & EDGE_TRUE_VALUE)
+	gimple_cond_make_false (stmt);
+      else if (e->flags & EDGE_FALSE_VALUE)
+	gimple_cond_make_true (stmt);
+      else
+	gcc_unreachable ();
+    }
+
+  return true;
+}
+
 /* A simple pass that attempts to fold all builtin functions.  This pass
    is run after we've propagated as many constants as we can.  */
 
@@ -2379,6 +2417,11 @@  execute_fold_all_builtins (void)
 		gsi_next (&i);
 		continue;
 
+	      case BUILT_IN_UNREACHABLE:
+		if (optimize_unreachable (i))
+		  cfg_changed = true;
+		break;
+
 	      case BUILT_IN_VA_START:
 	      case BUILT_IN_VA_END:
 	      case BUILT_IN_VA_COPY:
@@ -2393,6 +2436,9 @@  execute_fold_all_builtins (void)
 		continue;
 	      }
 
+	  if (result == NULL_TREE)
+	    break;
+
 	  if (dump_file && (dump_flags & TDF_DETAILS))
 	    {
 	      fprintf (dump_file, "Simplified\n  ");