ddpagan 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'.
----------------
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.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D157197/new/
https://reviews.llvm.org/D157197
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
