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 see why we need `int` and `size_t` but I'm not sure about the others. `void` is universally translated to `i8*` Adding a pointer to a type should be done in OMPKinds.def, something like: ``` #define __OMP_PTR_TYPE(NAME, BASE) OMP_TYPE(NAME, Base->getPointerTo()); __OMP_PTR_TYPE(VoidPtr, Int8) __OMP_PTR_TYPE(VoidPtrPtr, VoidPtr) ``` ================ Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:68 + InsertPointTy SaveIP() { return Builder.saveIP(); } + /// Callback type for variable finalization (think destructors). ---------------- The above can be committed separately as "wrap IRBuilder methods" or something, LGTM for that part. ================ Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:371 + /// threads copy the 'copyin' variables from Master copy to threadprivate + /// copies. + /// ---------------- Copy & paste, also below. ================ Comment at: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def:234 __OMP_RTL(__kmpc_critical, false, Void, IdentPtr, Int32, KmpCriticalNamePtrTy) -__OMP_RTL(__kmpc_critical_with_hint, false, Void, IdentPtr, Int32, KmpCriticalNamePtrTy, Int32) +__OMP_RTL(__kmpc_critical_with_hint, false, Void, IdentPtr, Int32, KmpCriticalNamePtrTy, IntPtrTy) __OMP_RTL(__kmpc_end_critical, false, Void, IdentPtr, Int32, KmpCriticalNamePtrTy) ---------------- I think this was correct before: ``` KMP_EXPORT void __kmpc_critical_with_hint(ident_t *, kmp_int32 global_tid, kmp_critical_name *, uint32_t hint); ``` ================ Comment at: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def:240 +__OMP_RTL(__kmpc_threadprivate_cached, false, VoidPtr, IdentPtr, Int32, VoidPtr, SizeTy, Void3Ptr) +__OMP_RTL(__kmpc_threadprivate_register, false, Void, IdentPtr, VoidPtr, KmpcCtorTy, KmpcCopyCtorTy, KmpcDtorTy) + ---------------- Can you add attributes (below) for these and call them in `llvm/test/Transforms/OpenMP/add_attributes.ll` [if not already present]. These changes and the additional types should go in a separate patch. ================ Comment at: llvm/lib/Frontend/OpenMP/OMPConstants.cpp:122 + } +} + ---------------- (I doubt we need the `initVoidPtr` function but if we do, the condition looks wrong) ================ Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:594 + // PRegPreFiniTI->getSuccessor(0) == PRegExitBB && + // "Unexpected CFG structure!"); ---------------- Preferably we would have a modified assertion. If that is too cumbersome, we can probably remove it. ================ Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:930 + InsertPointTy IP, Value *MasterAddr, Value *PrivateAddr, + llvm::IntegerType *IntPtrTy, bool BranchtoEnd) { + if (!IP.isSet()) ---------------- Can you add a comment here, maybe some ascii art, describing what we generate. ================ Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1013 + return Builder.CreateCall(Fn, Args); +} + ---------------- I think we should have a unit test for each of these. Style: drop the `llvm::`, not needed. 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