[PATCH] D91054: [Clang][OpenMP] Frontend work for sections - D89671

fghanim added a comment. > This is generally fine with me, @fghanim @Meinersbur any concerns? I have none. All good for me Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91054/new/ https://reviews.llvm.org/D91054

[PATCH] D91054: [Clang][OpenMP] Frontend work for sections - D89671

fghanim added a comment. You can update the tests as long as long as the output is correct. for example the difference is only in names, ordering of basicblocks and instructions that doesn't affect correctness, etc. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D91054: [Clang][OpenMP] Frontend work for sections - D89671

fghanim added a comment. Ping. Please add the Lit test for this. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91054/new/ https://reviews.llvm.org/D91054 ___ cfe-commits mailing list

[PATCH] D92189: [OpenMPIRBuilder] forward arguments as pointers to outlined function

fghanim added a comment. Thanks; just two more mediocre things if possible. if not. you are good to go. :) @jdoerfert In D92189#2424677 , @jdoerfert wrote: > In D92189#2424216 , @fghanim wrote: > >> To be

[PATCH] D91054: [Clang][OpenMP] Frontend work for sections - D89671

fghanim added a comment. Thanks for doing this. Please add proper lit test. you can find that under `clang/test/OpenMP/sections_codegen.cpp`. add a case for using the OMPBuilder. As an example look in the same directory for `parallel_codegen.cpp` , `master_codegen.cpp` , `critical_codegen.cpp`

[PATCH] D91109: [OMPIRBuilder] Start 'Create' methods with lower case. NFC.

fghanim accepted this revision. fghanim added a comment. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91109/new/ https://reviews.llvm.org/D91109 ___ cfe-commits mailing list

[PATCH] D79677: [Clang][OpenMP][OMPBuilder] (1/4) Privatize `parallel` for `OMPBuilder`

fghanim added a comment. In D79677#2248663 , @lebedev.ri wrote: > Tests missing I am not sure what to test here that isn't tested elsewhere in the series. This patch is the last in a series, and it represents the "usage" of the functionality added by

[PATCH] D85619: [clang][OpenMP][OMPBuilder] Use OMPBuilder to CG `omp single`

fghanim added a comment. In D85619#2248195 , @kiranchandramohan wrote: > What is the plan for this patch? Waiting on you to review it ;) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85619/new/

[PATCH] D79677: [Clang][OpenMP][OMPBuilder] (1/4) Privatize `parallel` for `OMPBuilder`

fghanim added a comment. In D79677#2248197 , @kiranchandramohan wrote: > What is the plan for this patch? To commit it ... eventually :) Once it (and the rest in the series) get reviewed. As it stands, I cannot commit this patch without the rest in

[PATCH] D85619: [clang][OpenMP][OMPBuilder] Use OMPBuilder to CG `omp single`

fghanim added a comment. ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85619/new/ https://reviews.llvm.org/D85619 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D85619: [clang][OpenMP][OMPBuilder] Use OMPBuilder to CG `omp single`

fghanim added a comment. In D85619#2208425 , @kiranchandramohan wrote: > Before I start, Why is this change all new code and no modification or > deletion of existing Clang functionality for omp single/copyprivate? Because the OMPBuilder currently is

[PATCH] D85619: [clang][OpenMP][OMPBuilder] Use OMPBuilder to CG `omp single`

fghanim added a comment. Feel free to add other reviewers. Thanks. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D85619/new/ https://reviews.llvm.org/D85619 ___ cfe-commits mailing list

[PATCH] D85619: [clang][OpenMP][OMPBuilder] Use OMPBuilder to CG `omp single`

fghanim created this revision. Herald added subscribers: cfe-commits, aaron.ballman, guansong, yaxunl. Herald added a project: clang. fghanim requested review of this revision. Herald added a reviewer: jdoerfert. Herald added a subscriber: sstefan1. ADD support to allow `omp single` to be CG-ed

[PATCH] D82470: [OpenMP][IRBuilder] Support allocas in nested parallel regions

fghanim accepted this revision. fghanim added a comment. This revision is now accepted and ready to land. Great. Thank you! LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82470/new/ https://reviews.llvm.org/D82470

[PATCH] D82470: [OpenMP][IRBuilder] Support allocas in nested parallel regions

fghanim added inline comments. Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:433 - Builder.SetInsertPoint(OuterFn->getEntryBlock().getFirstNonPHI()); - AllocaInst *TIDAddr = Builder.CreateAlloca(Int32, nullptr, "tid.addr"); - AllocaInst *ZeroAddr =

[PATCH] D82470: [OpenMP][IRBuilder] Support allocas in nested parallel regions

fghanim added a comment. > I'll address the nits. Thanks :) > Unsure if that is all. depends on the comment about `AllocaBuilder` Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:433 - Builder.SetInsertPoint(OuterFn->getEntryBlock().getFirstNonPHI()); - AllocaInst

[PATCH] D82470: [OpenMP][IRBuilder] Support allocas in nested parallel regions

fghanim added a comment. Thanks for the update. Just a couple of Nits, and a quick note Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:288 struct OutlineInfo { -SmallVector Blocks; using PostOutlineCBTy = std::function; Nit: isn't

[PATCH] D82470: [OpenMP][IRBuilder] Support allocas in nested parallel regions

fghanim added inline comments. Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:282 + IRBuilder<> AllocaBuilder; + /// Map to remember source location strings jdoerfert wrote: > fghanim wrote: > > jdoerfert wrote: > > > fghanim wrote: > > > >

[PATCH] D82822: [OpenMP][FIX] Consistently use OpenMPIRBuilder if requested

fghanim added a comment. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82822/new/ https://reviews.llvm.org/D82822 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D79677: [Clang][OpenMP][OMPBuilder] (1/4) Privatize `parallel` for `OMPBuilder`

fghanim marked 3 inline comments as done. fghanim added a comment. In D79677#2119019 , @kiranchandramohan wrote: > Is the ordering of code generation for clauses important? > copyin -> firstprivate -> barrier -> private if we emitted a copyin, then

[PATCH] D82722: [OpenMP][IRBuilder] Support nested parallel regions

fghanim added a comment. OK. Thanks :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82722/new/ https://reviews.llvm.org/D82722 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D82470: [OpenMP][IRBuilder] Support allocas in nested parallel regions

fghanim requested changes to this revision. fghanim added inline comments. This revision now requires changes to proceed. Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:282 + IRBuilder<> AllocaBuilder; + /// Map to remember source location strings

[PATCH] D79676: [Clang][OpenMP][OMPBuilder] Moving OMP allocation and cache creation code to OMPBuilderCBHelpers

This revision was automatically updated to reflect the committed changes. Closed by commit rG80e15b4574f4: [Clang][OpenMP][OMPBuilder] Moving OMP allocation and cache creation code to… (authored by fghanim). Changed prior to commit: https://reviews.llvm.org/D79676?vs=269584=273983#toc

[PATCH] D82722: [OpenMP][IRBuilder] Support nested parallel regions

fghanim added a comment. Thanks for working on this. LGTM. Did you make any changes other than splitting from D82470 ? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D82722/new/ https://reviews.llvm.org/D82722

[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

fghanim closed this revision. fghanim added a comment. commited: rG82b8236cf248 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79675/new/ https://reviews.llvm.org/D79675

[PATCH] D82470: [OpenMP][IRBuilder] Support nested parallel regions

fghanim added inline comments. Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:282 + IRBuilder<> AllocaBuilder; + /// Map to remember source location strings jdoerfert wrote: > fghanim wrote: > > What's the benefit of this over just maintaining

[PATCH] D82470: [OpenMP][IRBuilder] Support nested parallel regions

fghanim added a comment. Thanks for the Patch. I have few questions to help me understand what's going on. Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:282 + IRBuilder<> AllocaBuilder; + /// Map to remember source location strings What's

[PATCH] D79677: [Clang][OpenMP][OMPBuilder] (1/4) Privatize `parallel` for `OMPBuilder`

fghanim added a comment. ping - please suggest reviewers I can add to review the clang side of things? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79677/new/ https://reviews.llvm.org/D79677 ___

[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

fghanim added a comment. Ping. Does this patch need further changes? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79675/new/ https://reviews.llvm.org/D79675 ___ cfe-commits mailing list

[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

fghanim added a comment. you are responding to a comment from 2 weeks ago, so let's just move on. I uploaded a new patch yesterday. You have any comments on this patch? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79675/new/

[PATCH] D81484: [Clang][OpenMP][OMPBuilder] (4/4) Privatize `parallel` for `OMPBuilder`

fghanim marked 6 inline comments as done. fghanim added inline comments. Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:1777 } + +bool CodeGenFunction::OMPBuilderCBHelpers::EmitOMPFirstprivateClause( Same as current implementation, with certain changes around

[PATCH] D81483: [Clang][OpenMP][OMPBuilder] (3/4) Privatize `parallel` for `OMPBuilder`

fghanim marked 3 inline comments as done. fghanim added inline comments. Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:1554 llvm::SmallVectorImpl &) {} +bool CodeGenFunction::OMPBuilderCBHelpers::EmitOMPCopyinClause( Note

[PATCH] D79677: [clang][OpenMP][OMPIRBuilder] Adding some Privatization clauses to OpenMP `parallel` Directive

fghanim added a comment. @jdoerfert Please suggest reviewer's for this, and I will add them to other clang related patches Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79677/new/ https://reviews.llvm.org/D79677

[PATCH] D79677: [clang][OpenMP][OMPIRBuilder] Adding some Privatization clauses to OpenMP `parallel` Directive

fghanim updated this revision to Diff 269589. fghanim added a comment. Herald added subscribers: aaron.ballman, sstefan1. - rebase - splitting patch into 4 ( this, D81482 , D81483 , D81484 ) -

[PATCH] D81484: [Clang][OpenMP][OMPBuilder] (4/4) Privatize `parallel` for `OMPBuilder`

fghanim created this revision. fghanim added a reviewer: jdoerfert. Herald added subscribers: cfe-commits, sstefan1, guansong, yaxunl. Herald added a project: clang. fghanim added a child revision: D79677: [clang][OpenMP][OMPIRBuilder] Adding some Privatization clauses to OpenMP `parallel`

[PATCH] D81482: [Clang][OpenMP][OMPBuilder] (2/4) Privatize `parallel` for `OMPBuilder`

fghanim created this revision. fghanim added a reviewer: jdoerfert. Herald added subscribers: cfe-commits, sstefan1, guansong, yaxunl. Herald added a project: clang. fghanim added a child revision: D79677: [clang][OpenMP][OMPIRBuilder] Adding some Privatization clauses to OpenMP `parallel`

[PATCH] D81483: [Clang][OpenMP][OMPBuilder] (3/4) Privatize `parallel` for `OMPBuilder`

fghanim created this revision. fghanim added a reviewer: jdoerfert. Herald added subscribers: cfe-commits, aaron.ballman, sstefan1, guansong, yaxunl. Herald added a project: clang. fghanim added a child revision: D79677: [clang][OpenMP][OMPIRBuilder] Adding some Privatization clauses to OpenMP

[PATCH] D79676: [Clang][OpenMP][OMPBuilder] Moving OMP allocation and cache creation code to OMPBuilderCBHelpers

fghanim updated this revision to Diff 269584. fghanim added a comment. Herald added a subscriber: sstefan1. - rebase - addressing reviewer's comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79676/new/ https://reviews.llvm.org/D79676 Files:

[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

fghanim updated this revision to Diff 269583. fghanim added a comment. Herald added a subscriber: aaron.ballman. - Rebase + refactor based on D80222 - addressed reviewer comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

fghanim marked an inline comment as done. fghanim added a comment. I am moving on because we are not getting anywhere. However, There are few things I need to point out very quickly. > I fail to see the point in committing for example your target type solution > if we found a more generic

[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

fghanim marked 2 inline comments as done. fghanim added a comment. I am going to omit parts of the quote, because who wants to look at a wall of test - readability is important ;) In D79675#2051079 , @jdoerfert wrote: > In D79675#2047154

[PATCH] D80222: Replace Clang's createRuntimeFunction with the definitions in OMPKinds.def

fghanim added inline comments. Comment at: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def:244 -// TODO: Replace this with the real size_t type -#define __OMP_SIZE_TYPE(NAME) OMP_TYPE(NAME, Type::getInt64Ty(Ctx)) +#define __OMP_SIZE_TYPE(NAME) OMP_TYPE(NAME,

[PATCH] D80222: Replace Clang's createRuntimeFunction with the definitions in OMPKinds.def

fghanim added inline comments. Comment at: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def:244 -// TODO: Replace this with the real size_t type -#define __OMP_SIZE_TYPE(NAME) OMP_TYPE(NAME, Type::getInt64Ty(Ctx)) +#define __OMP_SIZE_TYPE(NAME) OMP_TYPE(NAME,

[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

fghanim updated this revision to Diff 265293. fghanim added a comment. Addressing more reviewers comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79675/new/ https://reviews.llvm.org/D79675 Files: clang/lib/CodeGen/CodeGenModule.cpp

[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

fghanim marked an inline comment as done. fghanim added inline comments. Comment at: llvm/lib/Frontend/OpenMP/OMPConstants.cpp:86 + llvm::omp::types::Int8PtrPtr = Int8Ptr->getPointerTo(); + llvm::omp::types::Int8PtrPtrPtr = Int8PtrPtr->getPointerTo(); +

[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

fghanim marked 10 inline comments as done. fghanim added a comment. In D79675#2045826 , @jdoerfert wrote: > In D79675#2045563 , @fghanim wrote: > > > So this whole thing was about moving Def.s out of

[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

fghanim added a comment. In D79675#2045405 , @jdoerfert wrote: > > Could you please list the other patches that are being held back by this > > one? I'd be interested to have a look at them. :) > > We need the target type support for D80222

[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

fghanim updated this revision to Diff 265071. fghanim marked 2 inline comments as done. fghanim added a comment. Addressing reviewer Comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79675/new/ https://reviews.llvm.org/D79675 Files:

[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

fghanim marked 21 inline comments as done. fghanim added a comment. In D79675#2044809 , @jdoerfert wrote: > What's the status? Can we split the target specific types stuff if this may > take a while, other patches depend on that :) Could you please

[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

fghanim marked 3 inline comments as done. fghanim added a comment. In D79675#2037314 , @jdoerfert wrote: > I left some comments on the type stuff. The rest looks good. > I think if you rebase the type stuff on D79739 >

[PATCH] D79677: [clang][OpenMP][OMPIRBuilder] Adding some Privatization clauses to OpenMP `parallel` Directive

fghanim marked 2 inline comments as done. fghanim added a comment. In D79677#2032942 , @jdoerfert wrote: > Generally you copied the existing Clang logic, correct? Well, Yes and no. I tried to keep as much as I can of the original implementation,

[PATCH] D79676: [Clang][OpenMP][OMPBuilder] Moving OMP allocation and cache creation code to OMPBuilderCBHelpers

fghanim marked 4 inline comments as done. fghanim added inline comments. Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:878 + [this, VDInit, OriginalAddr, VD, ThisFirstprivateIsLastprivate, + OrigVD, , IRef, ]() { // Emit private

[PATCH] D79676: [Clang][OpenMP][OMPBuilder] Moving OMP allocation and cache creation code to OMPBuilderCBHelpers

fghanim updated this revision to Diff 264012. fghanim marked an inline comment as done. fghanim added a comment. updating in response to review comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79676/new/ https://reviews.llvm.org/D79676

[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

fghanim updated this revision to Diff 264006. fghanim marked an inline comment as done. fghanim added a comment. - removed many Definitions from OMPKinds.def due to being added in D79739 - made changes based on reviewer comments - added unit test for

[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

fghanim marked 7 inline comments as done. fghanim added inline comments. Comment at: llvm/include/llvm/Frontend/OpenMP/OMPConstants.h:101 +extern Type *IntPtrTy; +extern Type *SizeTy; + jdoerfert wrote: > jdoerfert wrote: > > I can totally see why we need `int`

[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

fghanim marked 5 inline comments as done. fghanim added a comment. This is a small patch as it is. splitting it any further would make it very very small :D Comment at: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def:234 __OMP_RTL(__kmpc_critical, false, Void, IdentPtr,

[PATCH] D79677: [clang][OpenMP][OMPIRBuilder] Adding some Privatization clauses to OpenMP `parallel` Directive

fghanim created this revision. fghanim added a reviewer: jdoerfert. Herald added subscribers: cfe-commits, guansong, yaxunl. Herald added a project: clang. fghanim added a parent revision: D79676: [Clang][OpenMP][OMPBuilder] Moving OMP allocation and cache creation code to OMPBuilderCBHelpers. -

[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

fghanim created this revision. fghanim added a reviewer: jdoerfert. Herald added subscribers: llvm-commits, cfe-commits, guansong, hiraditya, yaxunl. Herald added projects: clang, LLVM. Added new methods to generate new runtime calls Added all required defenitions Added some target-specific

[PATCH] D79676: [Clang][OpenMP][OMPBuilder] Moving OMP allocation and cache creation code to OMPBuilderCBHelpers

fghanim created this revision. fghanim added a reviewer: jdoerfert. Herald added subscribers: cfe-commits, guansong, yaxunl. Herald added a project: clang. Modified the OMPBuilderCBHelpers in the following ways: - Moved location of class definition and deleted all constructors - Moved

[PATCH] D77112: [OpenMP][NFCI] Move OpenMP clause information to `lib/Frontend/OpenMP`

fghanim accepted this revision. fghanim added a comment. This revision is now accepted and ready to land. OK, As I said a few days ago, I went over the patch, and I didn't see any functional changes. I am accepting this patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D77112: [OpenMP][NFCI] Move OpenMP clause information to `lib/Frontend/OpenMP`

fghanim added a comment. Figured as much, just wanted to be sure. Anyways, this one also LGTM I'll wait a couple of days in case any one has comments, if not I'll approve it Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77112/new/

[PATCH] D77113: [OpenMP][NFC] Move and simplify directive -> allowed clause mapping

fghanim added a comment. Thanks for working on this as well. As an aside, I like the new allowed clause implementation much better. it is much simpler and cleaner than the previous one. I'll wait to see if anyone else has comments, but if not, then it LGTM. Repository: rG LLVM Github

[PATCH] D77112: [OpenMP][NFCI] Move OpenMP clause information to `lib/Frontend/OpenMP`

fghanim added a comment. Thanks for doing this. I looked at all of it, and the changes seem to be you just moved things to llvm/Frontend, and fixed namespaces/includes to work correctly with the new location. Is there here anything else I am missing? Repository: rG LLVM Github Monorepo

[PATCH] D75591: [OpenMP] Add firstprivate as a default data-sharing attribute to clang

fghanim added a comment. My comments were nits so. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75591/new/ https://reviews.llvm.org/D75591 ___ cfe-commits mailing list

[PATCH] D75591: [OpenMP] Add firstprivate as a default data-sharing attribute to clang

fghanim added a comment. @jdoerfert To answer your question as to why it is being codegened as shared, codegen doesn't handle `default` clause- at least I didn't come across such a thing- and if you think about it, it shouldn't need to. You either have `none` or `shared`: - `none` is largly

[PATCH] D75591: [OpenMP] Add firstprivate as a default data-sharing attribute to clang

fghanim added a comment. Done with `private` `firstprivate` `copyin` - working on modifying the clang test, and fixing some bugs. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75591/new/ https://reviews.llvm.org/D75591

[PATCH] D73285: [OpenMP][OMPIRBuilder][BugFix] Handle Unreachable Finalization blocks in `parallel` generation

fghanim abandoned this revision. fghanim added a comment. The bug this revision attempted to fix has been resolved as part of patch D74562 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73285/new/

[PATCH] D74562: [OpenMP][OMPIRBuilder] Introducing the `OMPBuilderCBHelpers` helper class

fghanim added a comment. I am done updating this patch. I still don't have commit access, I'd appreciate it if you'd commit this for me when you get a chance. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74562/new/

[PATCH] D74562: [OpenMP][OMPIRBuilder] Introducing the `OMPBuilderCBHelpers` helper class

fghanim updated this revision to Diff 245227. fghanim added a comment. Marking a call void Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74562/new/ https://reviews.llvm.org/D74562 Files: clang/lib/CodeGen/CGStmtOpenMP.cpp

[PATCH] D74562: [OpenMP][OMPIRBuilder] Introducing the `OMPBuilderCBHelpers` helper class

fghanim updated this revision to Diff 245151. fghanim added a comment. addressing review comments - Adding a comment to explain minor change. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D74562/new/ https://reviews.llvm.org/D74562 Files:

[PATCH] D74562: [OpenMP][OMPIRBuilder] Introducing the `OMPBuilderCBHelpers` helper class

fghanim added a comment. Regarding the third comment (which was removed for some reason), This new update should fix that Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:3134 // TODO: Replace with a generic helper function for emitting body auto BodyGenCB =

[PATCH] D74562: [OpenMP][OMPIRBuilder] Introducing the `OMPBuilderCBHelpers` helper class

fghanim updated this revision to Diff 245014. fghanim marked 2 inline comments as done. fghanim added a comment. Fixed bug where variables where still being allocated in original function entry block Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D74562: [OpenMP][OMPIRBuilder] Introducing the `OMPBuilderCBHelpers` helper class

fghanim created this revision. fghanim added a reviewer: jdoerfert. Herald added subscribers: cfe-commits, guansong. Herald added a project: clang. This patch introduces a new helper class `OMPBuilderCBHelpers`, which will contain all reusable C/C++ language specific function- alities required

[PATCH] D72304: [OpenMP][OMPIRBuilder] Add Directives (master and critical) to OMPBuilder.

fghanim updated this revision to Diff 244249. fghanim added a comment. Remove unnecessary preservation of builder insertion point in `emitCommonDirectiveExit` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72304/new/

[PATCH] D74372: [OpenMP][IRBuilder] Perform finalization (incl. outlining) late

fghanim added inline comments. Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:113 + if (llvm::OpenMPIRBuilder *OMPBuilder = CGM.getOpenMPIRBuilder()) +OMPBuilder->finalize(); } jdoerfert wrote: > fghanim wrote: > > jdoerfert wrote: > > > fghanim wrote:

[PATCH] D74372: [OpenMP][IRBuilder] Perform finalization (incl. outlining) late

fghanim added inline comments. Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:113 + if (llvm::OpenMPIRBuilder *OMPBuilder = CGM.getOpenMPIRBuilder()) +OMPBuilder->finalize(); } jdoerfert wrote: > fghanim wrote: > > Does this mean that `finalize()` is

[PATCH] D74372: [OpenMP][IRBuilder] Perform finalization (incl. outlining) late

fghanim added inline comments. Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:113 + if (llvm::OpenMPIRBuilder *OMPBuilder = CGM.getOpenMPIRBuilder()) +OMPBuilder->finalize(); } Does this mean that `finalize()` is being called twice everytime, here and

[PATCH] D72304: [OpenMP][OMPIRBuilder] Add Directives (master and critical) to OMPBuilder.

fghanim updated this revision to Diff 243839. fghanim added a comment. fixing unused variable. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72304/new/ https://reviews.llvm.org/D72304 Files: clang/lib/CodeGen/CGStmtOpenMP.cpp

[PATCH] D72304: [OpenMP][OMPIRBuilder] Add Directives (master and critical) to OMPBuilder.

fghanim added a comment. ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72304/new/ https://reviews.llvm.org/D72304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D72304: [OpenMP][OMPIRBuilder] Add Directives (master and critical) to OMPBuilder.

fghanim updated this revision to Diff 242144. fghanim added a comment. Addressing errors/warnings from commit build Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72304/new/ https://reviews.llvm.org/D72304 Files:

[PATCH] D72304: [OpenMP][OMPIRBuilder] Add Directives (master and critical) to OMPBuilder.

fghanim added a comment. ping Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72304/new/ https://reviews.llvm.org/D72304 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D72304: [OpenMP][OMPIRBuilder] Add Directives (master and critical) to OMPBuilder.

fghanim added a comment. Everything is fine! I just cloned llvm from git hub, added this revision with `arc patch`. then: `cmake -G"Unix Makefiles" -DLLVM_ENABLE_PROJECTS="clang;openmp" -DCMAKE_BUILD_TYPE=Release -DLLVM_OPTIMIZED_TABLEGEN=1 -DLLVM_USE_LINKER=gold ../llvm-project/llvm` `make` ,

[PATCH] D73285: [OpenMP][OMPIRBuilder][BugFix] Handle Unreachable Finalization blocks in `parallel` generation

fghanim marked an inline comment as done. fghanim added inline comments. Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:661 +AfterIP = InsertPointTy(ForkBB, ForkBB->end()); + } jdoerfert wrote: > fghanim wrote: > > jdoerfert wrote: > > > Why do we

[PATCH] D72304: [OpenMP][OMPIRBuilder] Add Directives (master and critical) to OMPBuilder.

fghanim added a comment. Thanks for trying. It compiles fine with me. How can I reproduce the problems you're getting? is there a way to pack what you have and send it to me so I can have a look? Also, a second question, how trustworthy is harbormaster when it says something is buildable?

[PATCH] D73285: [OpenMP][OMPIRBuilder][BugFix] Handle Unreachable Finalization blocks in `parallel` generation

fghanim marked an inline comment as done. fghanim added inline comments. Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:661 +AfterIP = InsertPointTy(ForkBB, ForkBB->end()); + } jdoerfert wrote: > Why do we need all of this? Can't we just *not do

[PATCH] D73285: [OpenMP][OMPIRBuilder][BugFix] Handle Unreachable Finalization blocks in `parallel` generation

fghanim updated this revision to Diff 240949. fghanim added a comment. - Squashing all the commits Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73285/new/ https://reviews.llvm.org/D73285 Files: clang/lib/CodeGen/CGStmtOpenMP.cpp

[PATCH] D73285: [OpenMP][OMPIRBuilder][BugFix] Handle Unreachable Finalization blocks in `parallel` generation

fghanim updated this revision to Diff 240886. fghanim added a comment. Adding lit test to clang for testing the fix Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73285/new/ https://reviews.llvm.org/D73285 Files:

[PATCH] D73285: [OpenMP][OMPIRBuilder][BugFix] Handle Unreachable Finalization blocks in `parallel` generation

fghanim updated this revision to Diff 240451. fghanim added a comment. Adding a new unittest for the this fix. Thanks to JDoerfert for Writing and providing me with this patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73285/new/

[PATCH] D73285: [OpenMP][OMPIRBuilder][BugFix] Handle Unreachable Finalization blocks in `parallel` generation

fghanim updated this revision to Diff 239991. fghanim added a comment. - Cleaning up some leftover code. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73285/new/ https://reviews.llvm.org/D73285 Files: clang/lib/CodeGen/CGStmtOpenMP.cpp

[PATCH] D73285: [OpenMP][OMPIRBuilder][BugFix] Handle Unreachable Finalization blocks in `parallel` generation

fghanim created this revision. fghanim added a reviewer: jdoerfert. Herald added subscribers: llvm-commits, cfe-commits, guansong, hiraditya. Herald added projects: clang, LLVM. In some situations (e.g. `while(1);` ) the body block(s) will not contain a branch to the finalization block. In this

[PATCH] D72304: [OpenMP][OMPIRBuilder] Add Directives (master and critical) to OMPBuilder.

fghanim added a comment. No, I don't have commit privileges. I'd appreciate if you'd commit this for me. Thanks :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72304/new/ https://reviews.llvm.org/D72304

[PATCH] D72304: [OpenMP][OMPIRBuilder] Add Directives (master and critical) to OMPBuilder.

fghanim updated this revision to Diff 238858. fghanim added a comment. Addressing reviewer's comments regarding styling. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72304/new/ https://reviews.llvm.org/D72304 Files:

[PATCH] D72304: [OpenMP]{OMPIRBuilder] Add Directives (master and critical) to OMPBuilder.

fghanim marked an inline comment as done and an inline comment as not done. fghanim added a comment. Thanks for reviewing this. After I address the last two comments below, How do I merge with llvm using phab? I'd appreciate an llvm specific link if possible. Comment at:

[PATCH] D72304: [OpenMP]{OMPIRBuilder] Add Directives (master and critical) to OMPBuilder.

fghanim updated this revision to Diff 238591. fghanim marked 9 inline comments as done. fghanim added a comment. Addressing reviewer's comments - fixed styling and naming according to llvm conventions Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D72304: [OpenMP]{OMPIRBuilder] Add Directives (master and critical) to OMPBuilder.

fghanim updated this revision to Diff 238301. fghanim marked 25 inline comments as done. fghanim added a comment. Addressing reviewer comments - Added regression tests - styling - adding asserts and todo where needed. Also, Now `EmitInlinedRegion` will merge blocks where legal. Repository:

[PATCH] D72304: [OpenMP]{OMPIRBuilder] Add Directives (master and critical) to OMPBuilder.

fghanim marked 4 inline comments as done. fghanim added a comment. I am working on a patch. In the mean time ;) Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:3020 + if (AllocaIP.isSet()) +AllocaInsertPt = &*AllocaIP.getPoint(); + auto OldReturnBlock =

[PATCH] D72304: Summary: Add OpenMP Directives (master and critical) to OMPBuilder, and use them in clang.

fghanim marked 10 inline comments as done. fghanim added a comment. > Commits need a message that explains the change. > Also the "Summary" should not be in the commit title. Is this something for future commits or do you want me to change these? If the latter, how do I do that? > This adds

[PATCH] D72304: Summary: Add OpenMP Directives (master and critical) to OMPBuilder, and use them in clang.

fghanim marked 8 inline comments as done. fghanim added a comment. So I modified the patch based on your comments. I removed all the code that I intended for llvm pass writers for now. I will look at it again later, figure something to do with the whole switch-if-cascade thing, and will

[PATCH] D72304: Summary: Add OpenMP Directives (master and critical) to OMPBuilder, and use them in clang.

fghanim updated this revision to Diff 237149. fghanim added a comment. - Adding array types to OMPKinds.def. Inlining runtime function calls generation. reformatting with clang format Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D72304/new/

[PATCH] D72304: Summary: Add OpenMP Directives (master and critical) to OMPBuilder, and use them in clang.

fghanim marked 5 inline comments as done. fghanim added a comment. I don't think there is enough in this patch to split it into two or three patches. The main part is `EmitOMPInlinedRegion` which does all the heavy lifting. At this point, both create `Master` & `Critical` are almost wrappers

  1   2   >