diff mbox series

[omp,ftracer] Improve comment in ignore_bb_p

Message ID 584958f3-f7f1-fdf4-645a-df8476cea5e6@suse.de
State New
Headers show
Series [omp,ftracer] Improve comment in ignore_bb_p | expand

Commit Message

Tom de Vries Oct. 5, 2020, 2:20 p.m. UTC 
[ 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
diff mbox series

Patch

[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  <tdevries@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)