jdoerfert added a comment. In D79675#2032764 <https://reviews.llvm.org/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 part of why it takes me so long to do reviews. ================ Comment at: llvm/include/llvm/Frontend/OpenMP/OMPConstants.h:101 +extern Type *IntPtrTy; +extern Type *SizeTy; + ---------------- jdoerfert wrote: > 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) > ``` My proposed scheme for the pointers was integrated in D79739 and will be commited shortly. ================ 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) ---------------- fghanim wrote: > jdoerfert wrote: > > 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); > > ``` > Nop, it's supposed to be whatever `IntPtrTy` is in the frontend (i.e. 32 for > 32bit, 64 for 64bit). > > `IntPtrTy` is actually a union with `sizety` in `CodeGenModule` I'm confused. I copied the declaration above from the runtime. It doesn't contain an `IntPtrTy` or similar. I agree that `IntPtrTy` is machine dependent and we need your initializers. What I tried to say is that at least one declaration in the runtime has `__kmpc_critical_with_hint` with an int32 as last argument. That said, the runtime is not shy of incompatible declarations for functions. ================ Comment at: llvm/lib/Frontend/OpenMP/OMPConstants.cpp:122 + } +} + ---------------- fghanim wrote: > jdoerfert wrote: > > (I doubt we need the `initVoidPtr` function but if we do, the condition > > looks wrong) > Forgot to refractor when I changed names. > > Regarding `void*` , I am not sure how it is defined in fortran. That's why I > made it possible to initialize it separately. I see. Let's cross that bridge once we get to it. ================ Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1013 + return Builder.CreateCall(Fn, Args); +} + ---------------- fghanim wrote: > jdoerfert wrote: > > I think we should have a unit test for each of these. > > > > Style: drop the `llvm::`, not needed. > Aside from `CreateCopyinclauseblocks()`, I couldn't think of a unittest for > the others, all they do is create a runtime call based on the passed arg. > That's why I didn't do it already. Let's test that one then :) 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
