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: > > > > > 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? 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
