[ was: Re: [PATCH][omp, ftracer] Don't duplicate blocks in SIMT region ]
On 10/5/20 10:51 AM, Alexander Monakov wrote:
> 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?
Not explicitly, no. But it was sent two weeks ago, and all the review
comments were related to compile-time slow-down, which I deal with in
separate patches. So, based on that and the fact that this fixes a
problem for nvptx offloading currently triggering in the libgomp
test-suite, I decided to commit.
> It has some
> issues.
>
OK, thanks for the review.
>> --- 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. */
>
How about this?
Thanks,
- Tom
[omp, ftracer] Improve comment in ignore_bb_p
Improve comment in ignore_bb_p related to the group marked by
IFN_GOMP_SIMT_ENTER_ALLOC/IFN_GOMP_SIMT_EXIT.
gcc/ChangeLog:
2020-10-05 Tom de Vries <tdevr...@suse.de>
* tracer.c (ignore_bb_p): Improve comment.
---
gcc/tracer.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/gcc/tracer.c b/gcc/tracer.c
index 7f32ccb7e21..cdda535ce9d 100644
--- a/gcc/tracer.c
+++ b/gcc/tracer.c
@@ -112,11 +112,15 @@ ignore_bb_p (const_basic_block 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. */
+ /* An IFN_GOMP_SIMT_ENTER_ALLOC/IFN_GOMP_SIMT_EXIT pair marks a SESE
+ region. When entering the region, per-lane storage is allocated,
+ and non-uniform execution is started. When exiting the region,
+ non-uniform execution is stopped and per-lane storage is deallocated.
+ These calls can be duplicated safely, if the entire region is
+ duplicated, otherwise not. So, since we're not asked about such a
+ region here, conservatively return false.
+ The IFN_GOMP_SIMT_VOTE_ANY and SIMT_XCHG_* are part of such
+ a region, 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)
