diff mbox series

[RFC,gimple] Move can_duplicate_bb_p to gimple_can_duplicate_bb_p

Message ID 20201009133633.GA29506@delia
State New
Headers show
Series [RFC,gimple] Move can_duplicate_bb_p to gimple_can_duplicate_bb_p | expand

Commit Message

Tom de Vries Oct. 9, 2020, 1:36 p.m. UTC
Hi,

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".

Any comments?

Thanks,
- Tom

[gimple] Move can_duplicate_bb_p to gimple_can_duplicate_bb_p

gcc/ChangeLog:

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

	* tracer.c (cached_can_duplicate_bb_p): Use can_duplicate_block_p
	instead of can_duplicate_bb_p.
	(can_duplicate_insn_p, can_duplicate_bb_no_insn_iter_p): Move ...
	* tree-cfg.c: ... here.
	* tracer.c (can_duplicate_bb_p): Move ...
	* tree-cfg.c (gimple_can_duplicate_bb_p): here.
	* tree-cfg.h (can_duplicate_insn_p, can_duplicate_bb_no_insn_iter_p):
	Declare.

---
 gcc/tracer.c   | 61 +---------------------------------------------------------
 gcc/tree-cfg.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++-
 gcc/tree-cfg.h |  2 ++
 3 files changed, 56 insertions(+), 61 deletions(-)

Comments

Richard Biener Oct. 12, 2020, 7:15 a.m. UTC | #1
On Fri, 9 Oct 2020, Tom de Vries wrote:

> Hi,
> 
> 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".
> 
> Any comments?

In principle it's correct to move this to the CFG hook since there
now seem to be stmts that cannot be duplicated and thus we need
to implement can_duplicate_bb_p.

Some minor things below...

> Thanks,
> - Tom
> 
> [gimple] Move can_duplicate_bb_p to gimple_can_duplicate_bb_p
> 
> gcc/ChangeLog:
> 
> 2020-10-09  Tom de Vries  <tdevries@suse.de>
> 
> 	* tracer.c (cached_can_duplicate_bb_p): Use can_duplicate_block_p
> 	instead of can_duplicate_bb_p.
> 	(can_duplicate_insn_p, can_duplicate_bb_no_insn_iter_p): Move ...
> 	* tree-cfg.c: ... here.
> 	* tracer.c (can_duplicate_bb_p): Move ...
> 	* tree-cfg.c (gimple_can_duplicate_bb_p): here.
> 	* tree-cfg.h (can_duplicate_insn_p, can_duplicate_bb_no_insn_iter_p):
> 	Declare.
> 
> ---
>  gcc/tracer.c   | 61 +---------------------------------------------------------
>  gcc/tree-cfg.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>  gcc/tree-cfg.h |  2 ++
>  3 files changed, 56 insertions(+), 61 deletions(-)
> 
> diff --git a/gcc/tracer.c b/gcc/tracer.c
> index e1c2b9527e5..16b46c65b14 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.  */
> diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
> index 5caf3b62d69..a5677859ffc 100644
> --- a/gcc/tree-cfg.c
> +++ b/gcc/tree-cfg.c
> @@ -6208,11 +6208,63 @@ gimple_split_block_before_cond_jump (basic_block bb)
>  }
>  
>  
> +/* Return true if gimple stmt G can be duplicated.  */
> +bool
> +can_duplicate_insn_p (gimple *g)

Does this need to be exported?  Please name it
can_duplicate_stmt_p.  It's also incomplete given the
function below

> +{
> +  /* 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.  */
> +bool
> +can_duplicate_bb_no_insn_iter_p (const_basic_block bb)

Ehm, better can_duplicate_last_stmt_p which is what it does?

Does this need to be exported?

> +{
> +  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 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)
>  {

that is, why not inline both functions here?

> +  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;
>  }
>  
> diff --git a/gcc/tree-cfg.h b/gcc/tree-cfg.h
> index beb4997a61c..9b270615321 100644
> --- a/gcc/tree-cfg.h
> +++ b/gcc/tree-cfg.h
> @@ -117,6 +117,8 @@ extern basic_block gimple_switch_label_bb (function *, gswitch *, unsigned);
>  extern basic_block gimple_switch_default_bb (function *, gswitch *);
>  extern edge gimple_switch_edge (function *, gswitch *, unsigned);
>  extern edge gimple_switch_default_edge (function *, gswitch *);
> +extern bool can_duplicate_insn_p (gimple *);
> +extern bool can_duplicate_bb_no_insn_iter_p (const_basic_block);
>  
>  /* Return true if the LHS of a call should be removed.  */
>  
>
Tom de Vries Oct. 13, 2020, 3:36 p.m. UTC | #2
On 10/12/20 9:15 AM, Richard Biener wrote:
> On Fri, 9 Oct 2020, Tom de Vries wrote:
> 
>> Hi,
>>
>> 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".
>>
>> Any comments?
> 
> In principle it's correct to move this to the CFG hook since there
> now seem to be stmts that cannot be duplicated and thus we need
> to implement can_duplicate_bb_p.
> 
> Some minor things below...
> 
>> Thanks,
>> - Tom
>>
>> [gimple] Move can_duplicate_bb_p to gimple_can_duplicate_bb_p
>>
>> gcc/ChangeLog:
>>
>> 2020-10-09  Tom de Vries  <tdevries@suse.de>
>>
>> 	* tracer.c (cached_can_duplicate_bb_p): Use can_duplicate_block_p
>> 	instead of can_duplicate_bb_p.
>> 	(can_duplicate_insn_p, can_duplicate_bb_no_insn_iter_p): Move ...
>> 	* tree-cfg.c: ... here.
>> 	* tracer.c (can_duplicate_bb_p): Move ...
>> 	* tree-cfg.c (gimple_can_duplicate_bb_p): here.
>> 	* tree-cfg.h (can_duplicate_insn_p, can_duplicate_bb_no_insn_iter_p):
>> 	Declare.
>>
>> ---
>>  gcc/tracer.c   | 61 +---------------------------------------------------------
>>  gcc/tree-cfg.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++-
>>  gcc/tree-cfg.h |  2 ++
>>  3 files changed, 56 insertions(+), 61 deletions(-)
>>
>> diff --git a/gcc/tracer.c b/gcc/tracer.c
>> index e1c2b9527e5..16b46c65b14 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.  */
>> diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
>> index 5caf3b62d69..a5677859ffc 100644
>> --- a/gcc/tree-cfg.c
>> +++ b/gcc/tree-cfg.c
>> @@ -6208,11 +6208,63 @@ gimple_split_block_before_cond_jump (basic_block bb)
>>  }
>>  
>>  
>> +/* Return true if gimple stmt G can be duplicated.  */
>> +bool
>> +can_duplicate_insn_p (gimple *g)
> 
> Does this need to be exported?

Yes, it's still used in tracer.c.  With the renaming, that has become
evident now.

>  Please name it
> can_duplicate_stmt_p.

Done.

>  It's also incomplete given the
> function below
> 

This is due to an attempt to keep checks that are specific for the last
stmt out of the loop for regular statements.

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?

Thanks,
- Tom

>> +{
>> +  /* 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.  */
>> +bool
>> +can_duplicate_bb_no_insn_iter_p (const_basic_block bb)
> 
> Ehm, better can_duplicate_last_stmt_p which is what it does?
> 
> Does this need to be exported?
> 
>> +{
>> +  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 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)
>>  {
> 
> that is, why not inline both functions here?
> 
>> +  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;
>>  }
>>  
>> diff --git a/gcc/tree-cfg.h b/gcc/tree-cfg.h
>> index beb4997a61c..9b270615321 100644
>> --- a/gcc/tree-cfg.h
>> +++ b/gcc/tree-cfg.h
>> @@ -117,6 +117,8 @@ extern basic_block gimple_switch_label_bb (function *, gswitch *, unsigned);
>>  extern basic_block gimple_switch_default_bb (function *, gswitch *);
>>  extern edge gimple_switch_edge (function *, gswitch *, unsigned);
>>  extern edge gimple_switch_default_edge (function *, gswitch *);
>> +extern bool can_duplicate_insn_p (gimple *);
>> +extern bool can_duplicate_bb_no_insn_iter_p (const_basic_block);
>>  
>>  /* Return true if the LHS of a call should be removed.  */
>>  
>>
>
Richard Biener Oct. 14, 2020, 6:15 a.m. UTC | #3
On Tue, 13 Oct 2020, Tom de Vries wrote:

> On 10/12/20 9:15 AM, Richard Biener wrote:
> > On Fri, 9 Oct 2020, Tom de Vries wrote:
> > 
> >> Hi,
> >>
> >> 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".
> >>
> >> Any comments?
> > 
> > In principle it's correct to move this to the CFG hook since there
> > now seem to be stmts that cannot be duplicated and thus we need
> > to implement can_duplicate_bb_p.
> > 
> > Some minor things below...
> > 
> >> Thanks,
> >> - Tom
> >>
> >> [gimple] Move can_duplicate_bb_p to gimple_can_duplicate_bb_p
> >>
> >> gcc/ChangeLog:
> >>
> >> 2020-10-09  Tom de Vries  <tdevries@suse.de>
> >>
> >> 	* tracer.c (cached_can_duplicate_bb_p): Use can_duplicate_block_p
> >> 	instead of can_duplicate_bb_p.
> >> 	(can_duplicate_insn_p, can_duplicate_bb_no_insn_iter_p): Move ...
> >> 	* tree-cfg.c: ... here.
> >> 	* tracer.c (can_duplicate_bb_p): Move ...
> >> 	* tree-cfg.c (gimple_can_duplicate_bb_p): here.
> >> 	* tree-cfg.h (can_duplicate_insn_p, can_duplicate_bb_no_insn_iter_p):
> >> 	Declare.
> >>
> >> ---
> >>  gcc/tracer.c   | 61 +---------------------------------------------------------
> >>  gcc/tree-cfg.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++-
> >>  gcc/tree-cfg.h |  2 ++
> >>  3 files changed, 56 insertions(+), 61 deletions(-)
> >>
> >> diff --git a/gcc/tracer.c b/gcc/tracer.c
> >> index e1c2b9527e5..16b46c65b14 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.  */
> >> diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
> >> index 5caf3b62d69..a5677859ffc 100644
> >> --- a/gcc/tree-cfg.c
> >> +++ b/gcc/tree-cfg.c
> >> @@ -6208,11 +6208,63 @@ gimple_split_block_before_cond_jump (basic_block bb)
> >>  }
> >>  
> >>  
> >> +/* Return true if gimple stmt G can be duplicated.  */
> >> +bool
> >> +can_duplicate_insn_p (gimple *g)
> > 
> > Does this need to be exported?
> 
> Yes, it's still used in tracer.c.  With the renaming, that has become
> evident now.
> 
> >  Please name it
> > can_duplicate_stmt_p.
> 
> Done.
> 
> >  It's also incomplete given the
> > function below
> > 
> 
> This is due to an attempt to keep checks that are specific for the last
> stmt out of the loop for regular statements.
> 
> 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.

Richard.

> Thanks,
> - Tom
> 
> >> +{
> >> +  /* 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.  */
> >> +bool
> >> +can_duplicate_bb_no_insn_iter_p (const_basic_block bb)
> > 
> > Ehm, better can_duplicate_last_stmt_p which is what it does?
> > 
> > Does this need to be exported?
> > 
> >> +{
> >> +  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 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)
> >>  {
> > 
> > that is, why not inline both functions here?
> > 
> >> +  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;
> >>  }
> >>  
> >> diff --git a/gcc/tree-cfg.h b/gcc/tree-cfg.h
> >> index beb4997a61c..9b270615321 100644
> >> --- a/gcc/tree-cfg.h
> >> +++ b/gcc/tree-cfg.h
> >> @@ -117,6 +117,8 @@ extern basic_block gimple_switch_label_bb (function *, gswitch *, unsigned);
> >>  extern basic_block gimple_switch_default_bb (function *, gswitch *);
> >>  extern edge gimple_switch_edge (function *, gswitch *, unsigned);
> >>  extern edge gimple_switch_default_edge (function *, gswitch *);
> >> +extern bool can_duplicate_insn_p (gimple *);
> >> +extern bool can_duplicate_bb_no_insn_iter_p (const_basic_block);
> >>  
> >>  /* Return true if the LHS of a call should be removed.  */
> >>  
> >>
> > 
>
Tom de Vries Oct. 14, 2020, 10:05 a.m. UTC | #4
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
Richard Biener Oct. 14, 2020, 12:35 p.m. UTC | #5
On Wed, 14 Oct 2020, Tom de Vries wrote:

> 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?

OK.

Thanks,
Richard.
diff mbox series

Patch

diff --git a/gcc/tracer.c b/gcc/tracer.c
index e1c2b9527e5..16b46c65b14 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.  */
diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index 5caf3b62d69..a5677859ffc 100644
--- a/gcc/tree-cfg.c
+++ b/gcc/tree-cfg.c
@@ -6208,11 +6208,63 @@  gimple_split_block_before_cond_jump (basic_block bb)
 }
 
 
+/* Return true if gimple stmt G can be duplicated.  */
+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.  */
+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 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)
 {
+  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;
 }
 
diff --git a/gcc/tree-cfg.h b/gcc/tree-cfg.h
index beb4997a61c..9b270615321 100644
--- a/gcc/tree-cfg.h
+++ b/gcc/tree-cfg.h
@@ -117,6 +117,8 @@  extern basic_block gimple_switch_label_bb (function *, gswitch *, unsigned);
 extern basic_block gimple_switch_default_bb (function *, gswitch *);
 extern edge gimple_switch_edge (function *, gswitch *, unsigned);
 extern edge gimple_switch_default_edge (function *, gswitch *);
+extern bool can_duplicate_insn_p (gimple *);
+extern bool can_duplicate_bb_no_insn_iter_p (const_basic_block);
 
 /* Return true if the LHS of a call should be removed.  */