fghanim marked 5 inline comments as done. fghanim added a comment. This is a small patch as it is. splitting it any further would make it very very small :D
================ 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: > 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` ================ Comment at: llvm/lib/Frontend/OpenMP/OMPConstants.cpp:122 + } +} + ---------------- 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. ================ Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:594 + // PRegPreFiniTI->getSuccessor(0) == PRegExitBB && + // "Unexpected CFG structure!"); ---------------- jdoerfert wrote: > Preferably we would have a modified assertion. If that is too cumbersome, we > can probably remove it. I was going to remove it actually. When running Dtor over an array, clang emits some loop logic that makes this untrue. ================ Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:1013 + return Builder.CreateCall(Fn, Args); +} + ---------------- 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. 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