On Mon, 5 Oct 2020, Tom de Vries wrote:
> I've had to modify this patch in two ways:
> - the original test-case stopped failing, though not the
> minimized one, so I added that one as a test-case
> - only testing for ENTER_ALLOC and EXIT, and not explicitly for VOTE_ANY
> in ignore_bb_p also stopped working, so I've added that now.
>
> Re-tested and committed.
I don't understand, was the patch already approved somewhere? It has some
issues.
> --- a/gcc/tracer.c
> +++ b/gcc/tracer.c
> @@ -108,6 +108,24 @@ ignore_bb_p (const_basic_block bb)
> return true;
> }
>
> + 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.
What does "its group" stand for? It seems obviously copy-pasted from the
description of IFN_UNIQUE treatment, where it is even less clear what the
"group" is.
(I know what it means, but the comment is not explaining things well at all)
> + The IFN_GOMP_SIMT_VOTE_ANY is currently part of such a group,
> + so the same holds there, but it could be argued that the
> + IFN_GOMP_SIMT_VOTE_ANY could be generated after that group,
> + in which case it could be duplicated. */
No, something like that cannot be argued, as VOTE_ANY may have data
dependencies to storage that is deallocated by SIMT_EXIT. You seem to be
claiming something that is simply not possible with the current design.
> + 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)))
Hm? So you are leaving SIMT_XCHG_* be until the next testcase breaks?
> + return true;
> + }
> +
> return false;
> }
Alexander
