[PATCH] D94973: [clang][OpenMP] Use OpenMPIRBuilder for workshare loops.

[Joel E. Denny via Phabricator via cfe-commits]

jdenny added a comment.

@Meinersbur Sorry, I haven't gotten very far yet in reviewing this, but I'll try to work on this more soon.



================
Comment at: clang/include/clang/AST/StmtOpenMP.h:31
 
+/// Representation of an OpenMP canonical loop.
+///
----------------
I think it would be helpful to cite the OpenMP spec here.  Just version and section number.


================
Comment at: clang/include/clang/AST/StmtOpenMP.h:37
+/// The OMPCanonicalLoop AST node wraps a ForStmt or CXXRangeForStmt that is
+/// known for fulfill OpenMP's loop requirements.
+///
----------------



================
Comment at: clang/include/clang/AST/StmtOpenMP.h:41
+/// purposes:
+/// * Loop variable: The user-accessible variable with different value for each
+///   iteration.
----------------
I suggest the terms "loop user variable" and "loop iteration variable".  In particular, I find "loop counter" to be misleading because "counter" suggests to me it's always an integer, so it's easy to get it confused with the "logical iteration counter".  If you're using standard terminology that I'm not aware of, then never mind.


================
Comment at: clang/include/clang/AST/StmtOpenMP.h:43
+///   iteration.
+/// * Loop counter: The variable used to identify a loop iterations; for
+///   range-based for-statement, this is the hidden iterator '__begin'. For
----------------



================
Comment at: clang/include/clang/AST/StmtOpenMP.h:46
+///   other loops, it is identical to the loop variable. Must be a random-access
+///   iterator or integer type.
+/// * Logical iteration counter: Normalized loop counter starting at 0 and
----------------
Please list pointer too as I don't think C has an iterator concept.


================
Comment at: clang/include/clang/AST/StmtOpenMP.h:48
+/// * Logical iteration counter: Normalized loop counter starting at 0 and
+///   incrementing by one at each iterations. Allows abstracting over the type
+///   of the loop counter and is always an unsigned integer type appropriate to
----------------



================
Comment at: clang/include/clang/AST/StmtOpenMP.h:61
+/// random-access iterator. The OpenMPIRBuilder will use this information to
+/// convert the loop into simd-, workshare-, distribute-, taskloop etc. For
+/// compatibility with the non-OpenMPIRBuilder codegen path, an OMPCanonicalLoop
----------------
Are those hyphens intentional?


================
Comment at: clang/include/clang/AST/StmtOpenMP.h:92
+/// <code>
+///   [&,__begin](std::vector<std::string>::iterator &Result, size_t Logical) {
+///   Result = __begin + Logical; }
----------------
Why is `__begin` an explicit capture here but not for the distance function?


================
Comment at: clang/include/clang/AST/StmtOpenMP.h:94
+///   Result = __begin + Logical; }
+/// </code>
+class OMPCanonicalLoop : public Stmt {
----------------
Thanks for this careful documentation!


================
Comment at: clang/include/clang/AST/StmtOpenMP.h:101
+  enum {
+    LOOPY_STMT,
+    DISTANCE_FUNC,
----------------
Why "loopy" with a "y"?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94973