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

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

jdenny added a comment.

In D94973#2547383 <https://reviews.llvm.org/D94973#2547383>, @jdoerfert wrote:

> I have a single last comment/request. @jdenny I'll take it you finish the review and accept as you see fit.

@jdoerfert, if you've already reviewed everything and want to accept so this can move forward, I'm fine with that.  Otherwise, I'll review more as I find time.  There's a lot of code.



================
Comment at: clang/include/clang/AST/StmtOpenMP.h:45
+/// known before entering the loop and allow skipping to an arbitrary iteration.
+/// The OMPCanonicalLoop AST node wraps a ForStmt or CXXRangeForStmt that is
+/// known to fulfill OpenMP's canonical loop requirements.
----------------



================
Comment at: clang/include/clang/AST/StmtOpenMP.h:54
+///   for range-based for-statement, this is the hidden iterator '__begin'. For
+///   other loops, it is identical to the loop variable. Must be a random-access
+///   iterator, pointer or integer type.
----------------



================
Comment at: clang/include/clang/AST/StmtOpenMP.h:58
+///   incrementing by one at each iteration. Allows abstracting over the type
+///   of the loop counter and is always an unsigned integer type appropriate to
+///   represent the range of the loop counter variable. Its value corresponds to
----------------



================
Comment at: clang/include/clang/AST/StmtOpenMP.h:59
+///   of the loop counter and is always an unsigned integer type appropriate to
+///   represent the range of the loop counter variable. Its value corresponds to
+///   the logical iteration number in the OpenMP specification.
----------------



================
Comment at: clang/include/clang/AST/StmtOpenMP.h:97
+/// `std::vector<std::string>::iterator::difference_type` aka `ptrdiff_t`).
+/// Therefore, the distance function will be <code>
+///   [&](size_t &Result) { Result = __end - __begin; }
----------------



================
Comment at: clang/include/clang/AST/StmtOpenMP.h:166
+    assert((isa<ForStmt>(S) || isa<CXXForRangeStmt>(S)) &&
+           "Canonical loop must be a for loop (range-based or otherwise)");
+    SubStmts[LOOPY_STMT] = S;
----------------
To convert run-time errors into compile-time errors, what if `setLoopStmt` is overloaded to take either a `ForStmt` or `CXXForRangeStmt` instead of any `Stmt`?


================
Comment at: clang/include/clang/AST/StmtOpenMP.h:190
+
+  /// The function that compute the loop  user variable from a logical iteration
+  /// counter. Can be evaluated as first statement in the loop.
----------------



================
Comment at: clang/include/clang/AST/StmtOpenMP.h:196
+  /// value, step size) are captured by the closure. In particular, the initial
+  /// value of loop counter is captured by value to be unaffected by previous
+  /// iterations.
----------------



================
Comment at: clang/include/clang/AST/StmtOpenMP.h:92
+/// <code>
+///   [&,__begin](std::vector<std::string>::iterator &Result, size_t Logical) {
+///   Result = __begin + Logical; }
----------------
Meinersbur wrote:
> jdenny wrote:
> > Why is `__begin` an explicit capture here but not for the distance function?
> Because `__begin` is captured by-value, everything lese uses the `&`default capture. This is the loop counter variable is modified inside the loop body.
> Because `__begin` is captured by-value, everything lese uses the `&`default capture. This is the loop counter variable is modified inside the loop body.




================
Comment at: clang/include/clang/AST/StmtOpenMP.h:92
+/// <code>
+///   [&,__begin](std::vector<std::string>::iterator &Result, size_t Logical) {
+///   Result = __begin + Logical; }
----------------
jdenny wrote:
> Meinersbur wrote:
> > jdenny wrote:
> > > Why is `__begin` an explicit capture here but not for the distance function?
> > Because `__begin` is captured by-value, everything lese uses the `&`default capture. This is the loop counter variable is modified inside the loop body.
> > Because `__begin` is captured by-value, everything lese uses the `&`default capture. This is the loop counter variable is modified inside the loop body.
> 
> 
Ah, I now see there are comments about this in the code.  I think a brief note here too would help because it stands out in the example, but your call.


================
Comment at: clang/include/clang/AST/StmtOpenMP.h:101
+  enum {
+    LOOPY_STMT,
+    DISTANCE_FUNC,
----------------
Meinersbur wrote:
> jdenny wrote:
> > Why "loopy" with a "y"?
> `loopy` was meant to refer to the property of the statement (ForStmt, CXXForRangeStmt, potentially others such as WhileStmt, DoStmt, functions from `#include <algorithm>`, etc) instead of a specific loop node (such as OMPLoopDirective or OMPCanonicalLoop itself), although I did not apply this idea consistently. Do you prefer a plain 'LOOP_STMT'?
> 
Yes.  In my mind, "loopy" doesn't help with that distinction.  But your call.


================
Comment at: clang/include/clang/Sema/Sema.h:10486
+
+  /// Called for syntactical loops (ForStmt for CXXRangeForStmt) associated to
+  /// an OpenMP loop directive.
----------------



================
Comment at: clang/lib/Sema/SemaOpenMP.cpp:5301
+  // Capture the initial iterator which represents the LoopVar value at the
+  // zero's logical iteration. Since the original ForStmt/CXXRangeForStmt update
+  // it in every iteration, capture it by value before it is modified.
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94973