[PATCH] D81478: [OPENMP50]Codegen for scan directives in parallel for regions.

[Alexey Bataev via Phabricator via cfe-commits]

ABataev marked an inline comment as done.
ABataev added inline comments.


================
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:3627
+      emitScanBasedDirective(CGF, S, NumIteratorsGen, FirstGen, SecondGen);
+    } else {
+      OMPCancelStackRAII CancelRegion(CGF, OMPD_parallel_for, S.hasCancel());
----------------
jdoerfert wrote:
> ABataev wrote:
> > jdoerfert wrote:
> > > This looks pretty much like D81658, right? Could we avoid the duplication and maybe use a templated function (or something else)?
> > The duplication is quite small. Here we don't need to check for lastprivates update, we need to check for the cancellation region. I don't think that the outlined functions are going to be much simpler and easier to read.
> I mean, these ~25 lines will exist 3 times at least. 2 times tacking lastprivate, which we can do for the third time at no extra cost, and 2 times with the cancelation RAII. Any change will need to be duplicated 3 times as well, etc.
> 
> If we do
> ```
> template<typename RAII>
> void emitWorksharingHelper(..., bool &HasLastprivate)
>     if (llvm::any_of(S.getClausesOfKind<OMPReductionClause>(),
>                      [](const OMPReductionClause *C) {
>                        return C->getModifier() == OMPC_REDUCTION_inscan;
>                      })) {
>       const auto &&NumIteratorsGen = [&S](CodeGenFunction &CGF) {
>         OMPLocalDeclMapRAII Scope(CGF);
>         OMPLoopScope LoopScope(CGF, S);
>         return CGF.EmitScalarExpr(S.getNumIterations());
>       };
>       const auto &&FirstGen = [&S](CodeGenFunction &CGF) {
>         RAII CancelRegion(CGF, OMPD_for, S.hasCancel());
>         (void)CGF.EmitOMPWorksharingLoop(S, S.getEnsureUpperBound(),
>                                          emitForLoopBounds,
>                                          emitDispatchForLoopBounds);
>         // Emit an implicit barrier at the end.
>         CGF.CGM.getOpenMPRuntime().emitBarrierCall(CGF, S.getBeginLoc(),
>                                                    OMPD_for);
>       };
>       const auto &&SecondGen = [&S, &HasLastprivate](CodeGenFunction &CGF) {
>         RAII CancelRegion(CGF, OMPD_for, S.hasCancel());
>         HasLastprivate = CGF.EmitOMPWorksharingLoop(S, S.getEnsureUpperBound(),
>                                          emitForLoopBounds,
>                                          emitDispatchForLoopBounds);
>       };
>       emitScanBasedDirective(CGF, S, NumIteratorsGen, FirstGen, SecondGen);
>     } else {
>       OMPCancelStackRAII CancelRegion(CGF, OMPD_parallel_for, S.hasCancel());
>         CGF.EmitOMPWorksharingLoop(S, S.getEnsureUpperBound(), emitForLoopBounds,
>                                    emitDispatchForLoopBounds);
>     }
> ```
> We can call it three times:
> ```
> emitWorksharingHelper<OMPCancelStackRAII>(...);
> emitWorksharingHelper<OMPCancelStackRAII>(...);
> emitWorksharingHelper<OMPDummyRAII>(...);
> ```
> 
> Wouldn't that be cleaner?
1. It requires the new `OMPDummyRAII` class.
2. Thу functionality under control of the `else` branch also must be affected.
3. We don't always need to check for `HasLastprivate`.
4. The `NumIteratorsGen` might be different.
5. We'll have a lot of trouble with adding new functionality to this function, especially if this functionality is unique for one construct and not required for others.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81478/new/

https://reviews.llvm.org/D81478