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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

In D79675#2045563 <https://reviews.llvm.org/D79675#2045563>, @fghanim wrote:

> In D79675#2045405 <https://reviews.llvm.org/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 <https://reviews.llvm.org/D80222>, D79739 <https://reviews.llvm.org/D79739> can go in but we need to modify it afterwards.
>
>
> 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.
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.
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.

> In any case, I addressed everything based on your earlier comments. Thanks for reviewing my patches. Let me know if you think other changes are needed here, otherwise could you please commit this for me, I still don't have commit access.

Thanks. Comments added.



================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:62
+
+  /// Set Insert point to Instruction /p IPII
+  void setInsertPoint(Instruction *IPII) { Builder.SetInsertPoint(IPII); }
----------------
Nit: \p not /p (Or does both work?)


================
Comment at: llvm/lib/Frontend/OpenMP/OMPConstants.cpp:86
+  llvm::omp::types::Int8PtrPtr = Int8Ptr->getPointerTo();
+  llvm::omp::types::Int8PtrPtrPtr = Int8PtrPtr->getPointerTo();
+
----------------
I think the macro way to specify pointer is easier to use.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPConstants.cpp:109
+  llvm::omp::types::SizeTy = (SizeTy) ? SizeTy : Int32;
+}
+
----------------
I guess we don't need anymore.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:104
+  initializeSizeTy(SizeTy);
+}
 
----------------
Probably won't need these either.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:61
     initializeRuntimeFunctions();
-    OMPBuilder.initialize();
+    OMPBuilder.initialize(Type::getInt32Ty(M.getContext()));
   }
----------------
Is size_t always 32 bit? I think you can use the datalayout from the module via `getIntPtrType`.


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:65
   OpenMPIRBuilder OMPBuilder(*M);
-  OMPBuilder.initialize();
+  OMPBuilder.initialize(Type::getInt32Ty(M->getContext()));
 
----------------
Same as above, also below.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79675/new/

https://reviews.llvm.org/D79675