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

alanphipps added a comment. In D94973#2617304 , @Meinersbur wrote: > Is is that same problem that D98265 > addresses? Yes, that appears to be the same. Thank you! Repository: rG LLVM Github Monorepo CHANGES SINCE

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

Meinersbur added a comment. Is is that same problem that D98265 addresses? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94973/new/ https://reviews.llvm.org/D94973 ___

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

alanphipps added a comment. Hello! I am seeing a downstream build failure on Mac as a result of your changes: clang/lib/CodeGen/CGStmtOpenMP.cpp:1916:3: error: too few template arguments for class template 'SmallVector' SmallVector EffectiveArgs; ^

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

Meinersbur added a comment. In D94973#2591493 , @jdenny wrote: >> I even think that representing semantic information alongside of syntax is >> one of the principles of Clang's AST. > > Interesting. Is that documented somewhere?

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

jdenny added a comment. In D94973#2591210 , @Meinersbur wrote: > In D94973#2590867 , @jdenny wrote: > >> One property of this patch that has bothered me is that OMPCanonicalLoop is >> not a loop. Instead, it's an

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

Meinersbur added a comment. In D94973#2590867 , @jdenny wrote: > One property of this patch that has bothered me is that OMPCanonicalLoop is > not a loop. Instead, it's an AST node that is sandwiched between a directive > and a loop to contain extra

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

jdenny added a comment. One property of this patch that has bothered me is that OMPCanonicalLoop is not a loop. Instead, it's an AST node that is sandwiched between a directive and a loop to contain extra information about the loop. The TreeTransform issues we've been discussing highlight

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

Meinersbur added inline comments. Comment at: clang/lib/Sema/TreeTransform.h:8321 +TreeTransform::TransformOMPCanonicalLoop(OMPCanonicalLoop *L) { + // The OMPCanonicalLoop will be recreated when transforming the loop-associted + // directive. jdenny wrote: >

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

jdenny accepted this revision. jdenny added a comment. This revision is now accepted and ready to land. Other than the additional comments I've requested, LGTM. Thanks for the explanations and the changes. Comment at: clang/lib/Sema/TreeTransform.h:8321

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

Meinersbur added inline comments. Comment at: clang/lib/Sema/TreeTransform.h:8321 +TreeTransform::TransformOMPCanonicalLoop(OMPCanonicalLoop *L) { + // The OMPCanonicalLoop will be recreated when transforming the loop-associted + // directive. jdenny wrote: >

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

jdenny added inline comments. Comment at: clang/lib/Sema/TreeTransform.h:8326-8331 +template +StmtResult +TreeTransform::TransformOMPCanonicalLoop(OMPCanonicalLoop *L) { + llvm_unreachable("OMPCanonicalLoop must be handled by the " + "OMPExecutableDirective

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

Meinersbur marked an inline comment as done. Meinersbur added a comment. In D94973#2575395 , @jdenny wrote: > This patch has no effect if the OpenMP IR builder is not enabled, and it's > disabled by default. Is that right? Yes, this is how it is

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

jdenny added a comment. This patch has no effect if the OpenMP IR builder is not enabled, and it's disabled by default. Is that right? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D94973/new/ https://reviews.llvm.org/D94973

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

jdenny added inline comments. Comment at: clang/lib/Sema/SemaOpenMP.cpp:5355 + Expr *Cond, *Inc; + VarDecl *CounterDecl, *LVDecl; + if (auto *For = dyn_cast(AStmt)) { jdenny wrote: > `CounterDecl` is the declaration of the "loop iteration variable" based on

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

Meinersbur added inline comments. Comment at: clang/lib/CodeGen/CodeGenFunction.h:292 + /// nest would extend. + SmallVector OMPLoopNestStack; + jdenny wrote: > Unless I missed something, the only accesses to `OMPLoopNestStack` are > `push_back`, `clear`,

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

jdenny added inline comments. Comment at: clang/lib/CodeGen/CodeGenFunction.h:292 + /// nest would extend. + SmallVector OMPLoopNestStack; + Unless I missed something, the only accesses to `OMPLoopNestStack` are `push_back`, `clear`, `back`, and `size`. It's

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

Meinersbur added inline comments. Comment at: clang/lib/Sema/SemaOpenMP.cpp:5215 + Expr *NegIncAmount = + AssertSuccess(Actions.BuildUnaryOp(nullptr, {}, UO_Minus, NewStep)); + Expr *BackwardDist = AssertSuccess( Meinersbur wrote: > jdenny

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

Meinersbur marked 6 inline comments as done. Meinersbur added a subscriber: rsmith. Meinersbur added inline comments. Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:94 // been "emitted" to the outside, thus, modifications are still sensible. - if

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

jdenny added inline comments. Comment at: clang/include/clang/AST/StmtOpenMP.h:166 +assert((isa(S) || isa(S)) && + "Canonical loop must be a for loop (range-based or otherwise)"); +SubStmts[LOOPY_STMT] = S; Meinersbur wrote: > jdenny wrote: > >

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

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

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

jdenny added a comment. In 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

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

jdoerfert added a comment. I have a single last comment/request. @jdenny I'll take it you finish the review and accept as you see fit. Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:986 + if (BodyGenCB) +BodyGenCB(CL->getBodyIP(), CL->getIndVar());

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

Meinersbur added inline comments. Comment at: clang/include/clang/AST/StmtOpenMP.h:41 +/// purposes: +/// * Loop variable: The user-accessible variable with different value for each +/// iteration. jdenny wrote: > I suggest the terms "loop user variable" and

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

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

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

Meinersbur added inline comments. Comment at: clang/lib/AST/Stmt.cpp:1275 -->isScalarType())) && -"captures by copy are expected to have a scalar type!"); break; jdoerfert wrote: > Why does this have

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

Meinersbur added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:17274 + if (IsTopScope && Kind != Sema::TryCapture_Implicit) { +ByRef = (Kind == Sema::TryCapture_ExplicitByRef); + } else if (S.getLangOpts().OpenMP && RSI->CapRegionKind == CR_OpenMP) {

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

jdoerfert added a comment. This seems sensible. The clang parts look OK but I haven't thought everything through to the last detail. I left some questions below, I guess we can go down this road and slowly transition to the new scheme. Comment at: clang/lib/AST/Stmt.cpp:1275

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

Meinersbur added a comment. ping 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

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

Meinersbur added a comment. ping 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

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

Meinersbur created this revision. Meinersbur added reviewers: jdoerfert, AMDChirag, anchu-rajendran, kiranchandramohan, SouraVX, ftynse, kiranktp, fghanim, ABataev. Meinersbur added projects: OpenMP, clang. Herald added subscribers: dexonsmith, rriddle, arphaman, guansong, hiraditya, yaxunl.