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: > > > 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. > > You're right in the sense that there is an assumption being made there. So > > would you recommend just staying with the semantics as defined in the spec > > regardless? > Yew, better to follow spec defaults. Ok. Sounds good. Thanks for review and suggestions, Alexey. 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
