fghanim marked 10 inline comments as done. fghanim added a comment. In D79675#2045826 <https://reviews.llvm.org/D79675#2045826>, @jdoerfert wrote:
> In D79675#2045563 <https://reviews.llvm.org/D79675#2045563>, @fghanim wrote: > > > So this whole thing was about moving Def.s out of `CGOMPRuntime`? Given how > > low priority making the-soon-to-be-deprecated `CGOMPRuntime` use the new > > Def.s is, and that I actually use these stuff as part of > > the-soon-to-be-the-way-to-CG-OMP `OMPBuilder`, wouldn't it have been better > > to postpone both patches until we are done with this one then add anything > > I didn't have already as part of D79739 <https://reviews.llvm.org/D79739> ? > > It would have certainly saved everyone a lot of time, and made more sense > > given that the earlier patch came out 2 days after mine, and the other > > patch today? :) > > > > > 1. Soon is relative. "Soon" is indeed relative, and so is "later", and so is 99% of the words. However, words have specific meanings, otherwise opposites would refer to the same thing, and words become useless and meaningless. "Soon" means soon. > 2. In the meantime people work on clang and add runtime functions into the > CGOMPRuntime but not into OMPKinds.def. We are playing catch up all the time, > thus wasting time. Until the `OMPBuilder` becomes the way to CG OMP IR, we will always be playing catch-up. All the `OMPKinds` def.s are very copy/paste-able one or two-liners and very easy to move. However, the actual code to CG the IR is not. Therefore, I hereby declare, that henceforth, for as long as I am working on the `OMPBuilder`, if people are willing to write the CG code of new things as part of the `OMPBuilder`, I am happy to be the one to move the def.s for them at anytime, including on my deathbed. :D > 3. I said it before, please split them up in small pieces. It does really not > help if we combine unrelated things in a single patch. It doesn't make it > faster and it is not less work at the end of the day. Johannes Comon .. This patch IS Small (~300 lines) and everything here is related (with one exception below). This patch adds 4 new `createXXXX` calls to the `OMPBuilder` needed by the privatization stuff in patches D79676 <https://reviews.llvm.org/D79676> and D79677 <https://reviews.llvm.org/D79677> . These calls require certain runtime calls and typing def.s. It is how we have always done it for any new `createXXXX` method starting with `CreateOMPParallel()`. The Def.s I added are almost completely gone. The exception I mentioned earlier is the pass-throughs to the `OMPBuilder`'s `IRBuilder` which were LGTM-ed early on and nothing uses them yet, so it is really a non-issue. However, What wasted everyone's time my friend, is removing integral parts to this patch which has two other feature patches depend on it, which meant I needed to build and rebuild to make sure things still work. it would have been way easier to make D79739 <https://reviews.llvm.org/D79739> modify and build on the typing in this one as I suggested there, and in retrospect, is something I should've pushed harder for. Anyways, let's move on. :D Let me know, if there are any further comments. ================ Comment at: llvm/lib/Frontend/OpenMP/OMPConstants.cpp:86 + llvm::omp::types::Int8PtrPtr = Int8Ptr->getPointerTo(); + llvm::omp::types::Int8PtrPtrPtr = Int8PtrPtr->getPointerTo(); + ---------------- jdoerfert wrote: > I think the macro way to specify pointer is easier to use. It is. But that patch has landed yet, and so I cannot use that. so for the time being, I am going to keep this way. and after both patches land, I'll make a minor patch that will just make this small modification. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D79675/new/ https://reviews.llvm.org/D79675 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
