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