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` 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. Kept a couple a Ineed in OMPConstants. will add them in OMPKinds.def after the other patch lands. ================ 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) ---------------- jdoerfert wrote: > 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. I cannot speak for what's in the runtime. However, in clang, this currently is defined to use `IntPtrTy`. If you go check, I have a todo in the current implementation for critical to come back and fix it. ================ 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) + ---------------- jdoerfert wrote: > 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. Added in D79739 ================ Comment at: llvm/lib/Frontend/OpenMP/OMPConstants.cpp:122 + } +} + ---------------- jdoerfert wrote: > 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. Modified it a bit and removed usage from initialization. If fortran people don't end using it, then we can remove it. 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
