Re: [PATCH] D21564: [OpenMP] Initial implementation of parse and sema for composite pragma 'distribute parallel for'
ABataev added inline comments. Comment at: include/clang/AST/StmtOpenMP.h:2884-2896 @@ +2883,15 @@ + + /// Increment expression for distribute loop (OMPLoopDirective contains + /// increment expression for #for loop) + Expr *DistIncExpr; + + /// \brief EnsureUpperBound for #pragma omp for: expression LB = PrevUB; + Expr *PrevEUBExpr; + + Expr *getDistInc() const { return DistIncExpr; } + Expr *getPrevEnsureUpperBound() const { return PrevEUBExpr; } + +protected: + void setDistInc(Expr *DistInc) { DistIncExpr = DistInc; } + void setPrevEnsureUpperBound(Expr *PrevEUB) { PrevEUBExpr = PrevEUB; } +}; Don't like the idea of public fields with protected setters. If these fields are required, they must be the part of the base OMPLoopDirective and reuse the logic for other helper expressions. Also, I believe, these fields are required for codegen. If so, it is better to add them in codegen patch Comment at: lib/CodeGen/CGStmtOpenMP.cpp:1868-1871 @@ -1867,1 +1867,6 @@ +void CodeGenFunction::EmitOMPDistributeParallelForDirective( +const OMPDistributeParallelForDirective &S) { + // TODO: codegen for distribute parallel for. +} + Could you just emit the captured statement as is, without dropping it? http://reviews.llvm.org/D21564 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21564: [OpenMP] Initial implementation of parse and sema for composite pragma 'distribute parallel for'
carlo.bertolli marked 2 inline comments as done. carlo.bertolli added a comment. Thanks very much for the quick review. I am about to update the patch reflecting your comments and to the latest trunk. Comment at: include/clang/AST/StmtOpenMP.h:2884-2896 @@ +2883,15 @@ + + /// Increment expression for distribute loop (OMPLoopDirective contains + /// increment expression for #for loop) + Expr *DistIncExpr; + + /// \brief EnsureUpperBound for #pragma omp for: expression LB = PrevUB; + Expr *PrevEUBExpr; + + Expr *getDistInc() const { return DistIncExpr; } + Expr *getPrevEnsureUpperBound() const { return PrevEUBExpr; } + +protected: + void setDistInc(Expr *DistInc) { DistIncExpr = DistInc; } + void setPrevEnsureUpperBound(Expr *PrevEUB) { PrevEUBExpr = PrevEUB; } +}; ABataev wrote: > Don't like the idea of public fields with protected setters. If these fields > are required, they must be the part of the base OMPLoopDirective and reuse > the logic for other helper expressions. > > Also, I believe, these fields are required for codegen. If so, it is better > to add them in codegen patch I removed the field and will add new fields to OMPLoopDirective later on when I produce a code gen patch. http://reviews.llvm.org/D21564 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21564: [OpenMP] Initial implementation of parse and sema for composite pragma 'distribute parallel for'
ABataev requested changes to this revision. This revision now requires changes to proceed. Comment at: lib/CodeGen/CGStmtOpenMP.cpp:1868-1871 @@ -1867,1 +1867,6 @@ +void CodeGenFunction::EmitOMPDistributeParallelForDirective( +const OMPDistributeParallelForDirective &S) { + EmitStmt(cast(S.getAssociatedStmt())->getCapturedStmt()); +} + No, it won't work. Use the following code: ``` OMPLexicalScope Scope(*this, S, /*AsInlined=*/true); CGM.getOpenMPRuntime().emitInlinedDirective( *this, OMPD_distribute_parallel_for, [&S](CodeGenFunction &CGF, PrePostActionTy &) { OMPLoopScope PreInitScope(CGF, S); CGF.EmitStmt(cast(S.getAssociatedStmt())->getCapturedStmt()); }); ``` Comment at: lib/Sema/SemaOpenMP.cpp:4985-4987 @@ +4984,5 @@ + + PrevLBDecl->setType(VType); + PrevUBDecl->setType(VType); + + // Previous lower and upper bounds are obtained from the region Not sure that this is a good solution. I'd prefer not to change the types of parameters. Maybe it is better to pass them as pointers and then cast to proper types? Repository: rL LLVM http://reviews.llvm.org/D21564 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21564: [OpenMP] Initial implementation of parse and sema for composite pragma 'distribute parallel for'
carlo.bertolli added a comment. Thanks for these further comments. I have just updated the diff applying the suggestions. Repository: rL LLVM http://reviews.llvm.org/D21564 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21564: [OpenMP] Initial implementation of parse and sema for composite pragma 'distribute parallel for'
ABataev accepted this revision. ABataev added a comment. This revision is now accepted and ready to land. LG Repository: rL LLVM http://reviews.llvm.org/D21564 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21564: [OpenMP] Initial implementation of parse and sema for composite pragma 'distribute parallel for'
ABataev added a comment. LG Comment at: lib/Sema/SemaOpenMP.cpp:1826-1827 @@ +1825,4 @@ +std::make_pair(".bound_tid.", KmpInt32PtrTy), +std::make_pair(".previous.lb.", KmpUInt64Ty), +std::make_pair(".previous.ub.", KmpUInt64Ty), +std::make_pair(StringRef(), QualType()) // __context with shared vars Just a small comment - I believe, it is better to use SizeTy, rather than KmpUInt64Ty. SizeTy is platform dependent and on 32 bit targets may result in more effective code. Repository: rL LLVM http://reviews.llvm.org/D21564 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21564: [OpenMP] Initial implementation of parse and sema for composite pragma 'distribute parallel for'
carlo.bertolli added a comment. Thanks for the hint - I have updated the diff to use Context.getSizeType(). Please let me know if this is what you meant. Repository: rL LLVM http://reviews.llvm.org/D21564 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21564: [OpenMP] Initial implementation of parse and sema for composite pragma 'distribute parallel for'
Carlo, yes this is what I meant. Best regards, Alexey Bataev Отправлено с iPhone > 24 июня 2016 г., в 18:16, Carlo Bertolli написал(а): > > carlo.bertolli added a comment. > > Thanks for the hint - I have updated the diff to use Context.getSizeType(). > Please let me know if this is what you meant. > > > Repository: > rL LLVM > > http://reviews.llvm.org/D21564 > > > ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21564: [OpenMP] Initial implementation of parse and sema for composite pragma 'distribute parallel for'
carlo.bertolli closed this revision. carlo.bertolli added a comment. Committed revision 273705. Repository: rL LLVM http://reviews.llvm.org/D21564 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
Re: [PATCH] D21564: [OpenMP] Initial implementation of parse and sema for composite pragma 'distribute parallel for'
carlo.bertolli added a comment. Resubmitted at revision 273884 after fixes. Repository: rL LLVM http://reviews.llvm.org/D21564 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits