[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:
> > > 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.
> > 1. It requires the new OMPDummyRAII class.
> 
> Sure, is it more than this?
> ```
> struct OMPDummyRAII {
>   OMPDummyRAII(...) {}
> };
> ```
> 
> > 2. Thу functionality under control of the else branch also must be affected.
> 
> Right: `sed -e/OMPCancelStackRAII/RAII/` :)
> 
> > 3. We don't always need to check for HasLastprivate.
> 
> There is no cost in doing so though. 
> 
> > 4. The NumIteratorsGen might be different.
> 
> Might be or is? Eyeballing this it looks like 3 times exact the same code.    
> 
> > 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.
> 
> What new functionality do you expect to be unique between them? If there is stuff planned, we might not want to unify the implementation, if it is just a hunch, we want to go ahead and have a single implemenation so it is easier to read and do global modifications. Finding out that something needs special handling is at the end of the day easier than finding all other (unrelated) locations that need to be changed.
> What new functionality do you expect to be unique between them? If there is stuff planned, we might not want to unify the implementation, if it is just a hunch, we want to go ahead and have a single implemenation so it is easier to read and do global modifications. Finding out that something needs special handling is at the end of the day easier than finding all other (unrelated) locations that need to be changed.

I have a different opinion here. When you will some specific implementation for one construct that is not required for other, you can easily miss it, and it might lead to unexpected results, like a compiler crash or something else. Adding step by step is much better, because you not only modify the code for the one particular construct, but also definitely will modify the tests for this construct. Having a common solution, you may easily miss something.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81478