[PATCH] D157197: [clang][CodeGen][OpenMP] Fix if-clause for 'target teams loop'

[Alexey Bataev via Phabricator via cfe-commits]

ABataev added inline comments.


================
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:1570-1575
+  // If we are here with a 'target teams loop' then we are emitting the
+  // 'parallel' region of the 'target teams distribute parallel for'
+  // emitted in place of the 'target teams loop'. Based on the specification
+  // noted above, an if-clause associated with a 'target teams loop', be it
+  // 'if(val)' or an 'if(target:val)', will apply only to 'target' and not
+  // the 'parallel' of the 'target teams distribute parallel for'.
----------------
ddpagan wrote:
> ABataev wrote:
> > ddpagan wrote:
> > > ABataev wrote:
> > > > It does not match the spec. 
> > > > ```
> > > > For a combined or composite construct, if no directive-name-modifier is specified then the if clause applies to all constituent constructs to which an if clause can apply.
> > > > ```
> > > > So, if(val) should be applied to both target and parallel regions, no?
> > > > It does not match the spec. 
> > > > ```
> > > > For a combined or composite construct, if no directive-name-modifier is specified then the if clause applies to all constituent constructs to which an if clause can apply.
> > > > ```
> > > > So, if(val) should be applied to both target and parallel regions, no?
> > > 
> > > Hi Alexey - Question for you: does revising the comment above at lines 1570-1575 to the following text help explain in a better way what's being done, and why?
> > > 
> > >   If we are handling a 'target teams distribute parallel for' explicitly written
> > >   in the source, and it has an 'if(val)' clause, the if condition is applied to
> > >   both 'target' and 'parallel' regions according to
> > >   OpenMP 5.2 [3.4, if Clause, Semantics, 15-18].
> > > 
> > >   However, if we are mapping an explicit 'target teams loop if(val)' onto a
> > >   'target teams distribute parallel for if(val)', to preserve the 'if' semantics
> > >   as specified by the user with the 'target teams loop', we apply it just to
> > >   the 'target' region.
> > It does not match the spec. Why we shall handle it this way?
> You're right, Alexey ... it doesn't match the spec, but here's why we thought this would be an appropriate way to implement 'target teams loop if(val)'. When a user specifies 'if(val)' with a 'target teams loop', their expectation is that its effect will only apply to the 'target' region. Since a 'loop' construct can be implemented in a number of different ways with the freedom granted by the specs description of 'loop' (part of which describes it as being able to be run concurrently), using a 'target teams distribute parallel for' construct is a simple and effective default choice, which is what happens today. 
>         target_teams_loop => target_teams_distribute_parallel_for
> Applying the if clause to the parallel region for this case can potentially limit it to one thread, which would hinder performance gains otherwise possible, and presumably wouldn't be what the user wanted/expected.
> 
> The semantics of the spec (OpenMP 5.2 [3.4, if Clause, Semantics, 15-18]) is definitely what should be applied to an explicit instance of target_teams_distribute_parallel_for, but in this case (when mapping target_teams_loop => target_teams_distribute_parallel_for) it seems reasonable to make the choice described above.
I'm not sure this is true users  expectations.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157197/new/

https://reviews.llvm.org/D157197