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