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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
