Re: [PATCH] D21564: [OpenMP] Initial implementation of parse and sema for composite pragma 'distribute parallel for'

[Carlo Bertolli via cfe-commits]

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

Re: [PATCH] D21564: [OpenMP] Initial implementation of parse and sema for composite pragma 'distribute parallel for'

[Carlo Bertolli via cfe-commits]

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'

[Alexey Bataev via cfe-commits]

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 via cfe-commits]

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'

[Alexey Bataev via cfe-commits]

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'

[Alexey Bataev via cfe-commits]

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'

[Carlo Bertolli via cfe-commits]

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'

[Alexey Bataev via cfe-commits]

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 ) {
+  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, [](CodeGenFunction , 
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 via cfe-commits]

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'

[Alexey Bataev via cfe-commits]

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 ) {
+  // 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