* [RFC][gimple] Move can_duplicate_bb_p to gimple_can_duplicate_bb_p
@ 2020-10-09 13:36 Tom de Vries
2020-10-12 7:15 ` Richard Biener
0 siblings, 1 reply; 6+ messages in thread
From: Tom de Vries @ 2020-10-09 13:36 UTC (permalink / raw)
To: gcc-patches; +Cc: Richard Biener
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(-)
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. */
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC][gimple] Move can_duplicate_bb_p to gimple_can_duplicate_bb_p
2020-10-09 13:36 [RFC][gimple] Move can_duplicate_bb_p to gimple_can_duplicate_bb_p Tom de Vries
@ 2020-10-12 7:15 ` Richard Biener
2020-10-13 15:36 ` Tom de Vries
0 siblings, 1 reply; 6+ messages in thread
From: Richard Biener @ 2020-10-12 7:15 UTC (permalink / raw)
To: Tom de Vries; +Cc: gcc-patches
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. */
>
>
--
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imend
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC][gimple] Move can_duplicate_bb_p to gimple_can_duplicate_bb_p
2020-10-12 7:15 ` Richard Biener
@ 2020-10-13 15:36 ` Tom de Vries
2020-10-14 6:15 ` Richard Biener
0 siblings, 1 reply; 6+ messages in thread
From: Tom de Vries @ 2020-10-13 15:36 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches
[-- Attachment #1: Type: text/plain, Size: 7993 bytes --]
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. */
>>
>>
>
[-- Attachment #2: 0001-gimple-Move-can_duplicate_bb_p-to-gimple_can_duplicate_bb_p.patch --]
[-- Type: text/x-patch, Size: 7048 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): Use can_duplicate_block_p
instead of can_duplicate_bb_p.
(analyze_bb): Use can_duplicate_insn_p.
(can_duplicate_insn_p, can_duplicate_bb_no_insn_iter_p): Move and
merge ...
tree-cfg.c (can_duplicate_stmt_p): ... here.
* tracer.c (can_duplicate_bb_p): Move ...
* tree-cfg.c (gimple_can_duplicate_bb_p): ... here.
* tree-cfg.h (can_duplicate_stmt_p): Declare.
---
gcc/tracer.c | 66 ++++------------------------------------------------------
gcc/tree-cfg.c | 49 ++++++++++++++++++++++++++++++++++++++++++-
gcc/tree-cfg.h | 1 +
3 files changed, 53 insertions(+), 63 deletions(-)
diff --git a/gcc/tracer.c b/gcc/tracer.c
index e1c2b9527e5..e335a33fcd1 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,13 +131,14 @@ 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);
+ gimple *last = last_stmt (CONST_CAST_BB (bb));
+ bool can_duplicate = !last || can_duplicate_stmt_p (last, false);
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);
+ can_duplicate = can_duplicate && can_duplicate_stmt_p (stmt, true);
}
*count = n;
cache_can_duplicate_bb_p (bb, can_duplicate);
diff --git a/gcc/tree-cfg.c b/gcc/tree-cfg.c
index 5caf3b62d69..2fd7daa4fda 100644
--- a/gcc/tree-cfg.c
+++ b/gcc/tree-cfg.c
@@ -6208,11 +6208,58 @@ gimple_split_block_before_cond_jump (basic_block bb)
}
+/* Return true if gimple stmt G can be duplicated. If !ASSUME_NOT_LAST_P,
+ also do checks that can only fail for the last insn in a block. */
+bool
+can_duplicate_stmt_p (gimple *g, bool assume_not_last_p)
+{
+ /* 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;
+
+ if (!assume_not_last_p)
+ {
+ /* 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 (bb->index < NUM_FIXED_BLOCKS)
+ return false;
+
+ gimple *last = last_stmt (CONST_CAST_BB (bb));
+ if (last && can_duplicate_stmt_p (last, false))
+ return false;
+
+ for (gimple_stmt_iterator gsi = gsi_start_bb (CONST_CAST_BB (bb));
+ !gsi_end_p (gsi); gsi_next (&gsi))
+ if (!can_duplicate_stmt_p (gsi_stmt (gsi)), true)
+ return false;
+
return true;
}
diff --git a/gcc/tree-cfg.h b/gcc/tree-cfg.h
index beb4997a61c..eadb3e6f440 100644
--- a/gcc/tree-cfg.h
+++ b/gcc/tree-cfg.h
@@ -117,6 +117,7 @@ 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_stmt_p (gimple *, bool = false);
/* Return true if the LHS of a call should be removed. */
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC][gimple] Move can_duplicate_bb_p to gimple_can_duplicate_bb_p
2020-10-13 15:36 ` Tom de Vries
@ 2020-10-14 6:15 ` Richard Biener
2020-10-14 10:05 ` Tom de Vries
0 siblings, 1 reply; 6+ messages in thread
From: Richard Biener @ 2020-10-14 6:15 UTC (permalink / raw)
To: Tom de Vries; +Cc: gcc-patches
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. */
> >>
> >>
> >
>
--
Richard Biener <rguenther@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imend
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC][gimple] Move can_duplicate_bb_p to gimple_can_duplicate_bb_p
2020-10-14 6:15 ` Richard Biener
@ 2020-10-14 10:05 ` Tom de Vries
2020-10-14 12:35 ` Richard Biener
0 siblings, 1 reply; 6+ messages in thread
From: Tom de Vries @ 2020-10-14 10:05 UTC (permalink / raw)
To: Richard Biener; +Cc: gcc-patches
[-- 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;
}
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC][gimple] Move can_duplicate_bb_p to gimple_can_duplicate_bb_p
2020-10-14 10:05 ` Tom de Vries
@ 2020-10-14 12:35 ` Richard Biener
0 siblings, 0 replies; 6+ messages in thread
From: Richard Biener @ 2020-10-14 12:35 UTC (permalink / raw)
To: Tom de Vries; +Cc: gcc-patches
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-10-14 12:35 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-09 13:36 [RFC][gimple] Move can_duplicate_bb_p to gimple_can_duplicate_bb_p Tom de Vries
2020-10-12 7:15 ` Richard Biener
2020-10-13 15:36 ` Tom de Vries
2020-10-14 6:15 ` Richard Biener
2020-10-14 10:05 ` Tom de Vries
2020-10-14 12:35 ` Richard Biener
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).