Re: [RFC][gimple] Move can_duplicate_bb_p to gimple_can_duplicate_bb_p

[Tom de Vries <tdevries@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 636 bytes --]

On 10/14/20 8:15 AM, Richard Biener wrote:
>> I've tried to address this by merging can_duplicate_stmt_p and
>> can_duplicate_last_stmt_p, and adding a default parameter.
>>
>> Better like this?
> Sorry for iterating again but since we now would appropriately
> handle things in the CFG hook there's no need for tracer.c to
> do this on its own via the _stmt calls.  So I suggest to
> remove the unifying of the stmt counting loop and use
> the can_duplicate_block_p CFG hook directly instead (but of course
> still cache its outcome).  That way we can simplify what is
> exported.

Np :) . Fully retested, OK for trunk?

Thanks,
- Tom

[-- Attachment #2: 0001-gimple-Move-can_duplicate_bb_p-to-gimple_can_duplicate_bb_p.patch --]
[-- Type: text/x-patch, Size: 5948 bytes --]

[gimple] Move can_duplicate_bb_p to gimple_can_duplicate_bb_p

The function gimple_can_duplicate_bb_p currently always returns true.

The presence of can_duplicate_bb_p in tracer.c however suggests that
there are cases when bb's indeed cannot be duplicated.

Move the implementation of can_duplicate_bb_p to gimple_can_duplicate_bb_p.

Bootstrapped and reg-tested on x86_64-linux.

Build x86_64-linux with nvptx accelerator and tested libgomp.

No issues found.

As corner-case check, bootstrapped and reg-tested a patch that makes
gimple_can_duplicate_bb_p always return false, resulting in
PR97333 - "[gimple_can_duplicate_bb_p == false, tree-ssa-threadupdate]
ICE in duplicate_block, at cfghooks.c:1093".

gcc/ChangeLog:

2020-10-09  Tom de Vries  <tdevries@suse.de>

	* tracer.c (cached_can_duplicate_bb_p, analyze_bb): Use
	can_duplicate_block_p.
	(can_duplicate_insn_p, can_duplicate_bb_no_insn_iter_p)
	(can_duplicate_bb_p): Move and merge ...
	* tree-cfg.c (gimple_can_duplicate_bb_p): ... here.

---
 gcc/tracer.c   | 66 +++-------------------------------------------------------
 gcc/tree-cfg.c | 38 ++++++++++++++++++++++++++++++++-
 2 files changed, 40 insertions(+), 64 deletions(-)

diff --git a/gcc/tracer.c b/gcc/tracer.c
index e1c2b9527e5..2f9daf92d79 100644
--- a/gcc/tracer.c
+++ b/gcc/tracer.c
@@ -84,65 +84,6 @@ bb_seen_p (basic_block bb)
   return bitmap_bit_p (bb_seen, bb->index);
 }
 
-/* Return true if gimple stmt G can be duplicated.  */
-static bool
-can_duplicate_insn_p (gimple *g)
-{
-  /* An IFN_GOMP_SIMT_ENTER_ALLOC/IFN_GOMP_SIMT_EXIT call must be
-     duplicated as part of its group, or not at all.
-     The IFN_GOMP_SIMT_VOTE_ANY and IFN_GOMP_SIMT_XCHG_* are part of such a
-     group, so the same holds there.  */
-  if (is_gimple_call (g)
-      && (gimple_call_internal_p (g, IFN_GOMP_SIMT_ENTER_ALLOC)
-	  || gimple_call_internal_p (g, IFN_GOMP_SIMT_EXIT)
-	  || gimple_call_internal_p (g, IFN_GOMP_SIMT_VOTE_ANY)
-	  || gimple_call_internal_p (g, IFN_GOMP_SIMT_XCHG_BFLY)
-	  || gimple_call_internal_p (g, IFN_GOMP_SIMT_XCHG_IDX)))
-    return false;
-
-  return true;
-}
-
-/* Return true if BB can be duplicated.  Avoid iterating over the insns.  */
-static bool
-can_duplicate_bb_no_insn_iter_p (const_basic_block bb)
-{
-  if (bb->index < NUM_FIXED_BLOCKS)
-    return false;
-
-  if (gimple *g = last_stmt (CONST_CAST_BB (bb)))
-    {
-      /* A transaction is a single entry multiple exit region.  It
-	 must be duplicated in its entirety or not at all.  */
-      if (gimple_code (g) == GIMPLE_TRANSACTION)
-	return false;
-
-      /* An IFN_UNIQUE call must be duplicated as part of its group,
-	 or not at all.  */
-      if (is_gimple_call (g)
-	  && gimple_call_internal_p (g)
-	  && gimple_call_internal_unique_p (g))
-	return false;
-    }
-
-  return true;
-}
-
-/* Return true if BB can be duplicated.  */
-static bool
-can_duplicate_bb_p (const_basic_block bb)
-{
-  if (!can_duplicate_bb_no_insn_iter_p (bb))
-    return false;
-
-  for (gimple_stmt_iterator gsi = gsi_start_bb (CONST_CAST_BB (bb));
-       !gsi_end_p (gsi); gsi_next (&gsi))
-    if (!can_duplicate_insn_p (gsi_stmt (gsi)))
-      return false;
-
-  return true;
-}
-
 static sbitmap can_duplicate_bb;
 
 /* Cache VAL as value of can_duplicate_bb_p for BB.  */
@@ -167,7 +108,7 @@ cached_can_duplicate_bb_p (const_basic_block bb)
       return false;
     }
 
-  return can_duplicate_bb_p (bb);
+  return can_duplicate_block_p (bb);
 }
 
 /* Return true if we should ignore the basic block for purposes of tracing.  */
@@ -190,16 +131,15 @@ analyze_bb (basic_block bb, int *count)
   gimple_stmt_iterator gsi;
   gimple *stmt;
   int n = 0;
-  bool can_duplicate = can_duplicate_bb_no_insn_iter_p (bb);
 
   for (gsi = gsi_start_bb (bb); !gsi_end_p (gsi); gsi_next (&gsi))
     {
       stmt = gsi_stmt (gsi);
       n += estimate_num_insns (stmt, &eni_size_weights);
-      can_duplicate = can_duplicate && can_duplicate_insn_p (stmt);
     }
   *count = n;
-  cache_can_duplicate_bb_p (bb, can_duplicate);
+
+  cache_can_duplicate_bb_p (bb, can_duplicate_block_p (CONST_CAST_BB (bb)));
 }
 
 /* Return true if E1 is more frequent than E2.  */
diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index 5caf3b62d69..002560d9370 100644
--- a/gcc/tree-cfg.c
+++ b/gcc/tree-cfg.c
@@ -6211,8 +6211,44 @@ gimple_split_block_before_cond_jump (basic_block bb)
 /* Return true if basic_block can be duplicated.  */
 
 static bool
-gimple_can_duplicate_bb_p (const_basic_block bb ATTRIBUTE_UNUSED)
+gimple_can_duplicate_bb_p (const_basic_block bb)
 {
+  gimple *last = last_stmt (CONST_CAST_BB (bb));
+
+  /* Do checks that can only fail for the last stmt, to minimize the work in the
+     stmt loop.  */
+  if (last) {
+    /* A transaction is a single entry multiple exit region.  It
+       must be duplicated in its entirety or not at all.  */
+    if (gimple_code (last) == GIMPLE_TRANSACTION)
+      return false;
+
+    /* An IFN_UNIQUE call must be duplicated as part of its group,
+       or not at all.  */
+    if (is_gimple_call (last)
+	&& gimple_call_internal_p (last)
+	&& gimple_call_internal_unique_p (last))
+      return false;
+  }
+
+  for (gimple_stmt_iterator gsi = gsi_start_bb (CONST_CAST_BB (bb));
+       !gsi_end_p (gsi); gsi_next (&gsi))
+    {
+      gimple *g = gsi_stmt (gsi);
+
+      /* An IFN_GOMP_SIMT_ENTER_ALLOC/IFN_GOMP_SIMT_EXIT call must be
+	 duplicated as part of its group, or not at all.
+	 The IFN_GOMP_SIMT_VOTE_ANY and IFN_GOMP_SIMT_XCHG_* are part of such a
+	 group, so the same holds there.  */
+      if (is_gimple_call (g)
+	  && (gimple_call_internal_p (g, IFN_GOMP_SIMT_ENTER_ALLOC)
+	      || gimple_call_internal_p (g, IFN_GOMP_SIMT_EXIT)
+	      || gimple_call_internal_p (g, IFN_GOMP_SIMT_VOTE_ANY)
+	      || gimple_call_internal_p (g, IFN_GOMP_SIMT_XCHG_BFLY)
+	      || gimple_call_internal_p (g, IFN_GOMP_SIMT_XCHG_IDX)))
+	return false;
+    }
+
   return true;
 }