[RFC][gimple] Move can_duplicate_bb_p to gimple_can_duplicate_bb_p
Tom de Vries
tdevries@suse.de
Tue Oct 13 15:36:07 GMT 2020
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. */
>>
>>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-gimple-Move-can_duplicate_bb_p-to-gimple_can_duplicate_bb_p.patch
Type: text/x-patch
Size: 7048 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/gcc-patches/attachments/20201013/8bb306df/attachment-0001.bin>
More information about the Gcc-patches
mailing list