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