[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] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

jdoerfert accepted this revision. jdoerfert added a comment. This revision is now accepted and ready to land. LGTM. Thanks a lot! Sorry for the delay in reviewing. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79675/new/

[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] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

jdoerfert added subscribers: raghavendhra, jhuber6, anchu-rajendran, kiranktp, DavidTruby, ronlieb, kiranchandramohan. jdoerfert added a comment. In D79675#2058657 , @fghanim wrote: > I am moving on because we are not getting anywhere. However, There are

[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

jdoerfert added a comment. In D79675#2051682 , @fghanim wrote: > > My goal is to save us time, during development, review, maintenance, and > > future extensions. I hope you know that. > > I am certain of that. However, I am starting to have doubts if my

[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] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

jdoerfert added a comment. In D79675#2047154 , @fghanim wrote: > In D79675#2045826 , @jdoerfert wrote: > > > In D79675#2045563 , @fghanim wrote: > > > > > So this whole

[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

jdoerfert added a comment. In D79675#2045563 , @fghanim wrote: > In D79675#2045405 , @jdoerfert wrote: > > > > Could you please list the other patches that are being held back by this > > > one? I'd be interested

[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

jdoerfert added a comment. > 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 , D79739 can go in but we need

[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

jdoerfert added a comment. Herald added a subscriber: sstefan1. What's the status? Can we split the target specific types stuff if this may take a while, other patches depend on that :) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79675/new/

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

jdoerfert added a comment. In D79675#2037484 , @fghanim wrote: > 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

[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] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

jdoerfert added a comment. I left some comments on the type stuff. The rest looks good. I think if you rebase the type stuff on D79739 (which I can merge) we should only need to expand `initializeTypes` to make this work as expected. WDYT?

[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

jdoerfert added a comment. In D79675#2032764 , @fghanim wrote: > This is a small patch as it is. splitting it any further would make it very > very small :D Very small is great, then I can accept it fast, finding enough time to read larger patches is

[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] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

jdoerfert added a comment. Thanks for these! We should probably split this in three patches. I commented below, mostly minor stuff. Comment at: llvm/include/llvm/Frontend/OpenMP/OMPConstants.h:101 +extern Type *IntPtrTy; +extern Type *SizeTy; + I can totally

[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