fghanim marked 21 inline comments as done. fghanim added a comment. In D79675#2044809 <https://reviews.llvm.org/D79675#2044809>, @jdoerfert wrote:
> What's the status? Can we split the target specific types stuff if this may > take a while, other patches depend on that :) 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. :) ================ 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: > > > fghanim wrote: > > > > 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. > > > That is just an existing bug in Clang. The runtime is consistent with the > > > type and arguably it is the runtime we must match, not the other way > > > around ;) > > Apparently, this used to be `uintptr_t` in the runtime and was changed by > > D51235 . It doesn't say why the change was made. > > > > Clang uses this in multiple places, which makes me think it may have been > > intentional. So I suggest we go by the standard. Where can I find what does > > the OMP standard say about how the signature of the runtime calls should > > look like? This way I'll fix this one accordingly, and later we may go and > > make sure that all the rest match up with the standard - where possible. > This is not a standard function. It is a runtime function. I suspect it is a > int32_t because enums default to `int` which is 32 bit on all interesting > platforms. Should probably be a `int` but I would need to read up to confirm. > Long story short, and as I said before, it is a bug in clang. The code here > was correct. > > FWIW, we will soon replace clang's runtime function generation with these > macros. That will happen in the next days I hope. Oh, my bad. I thought OMP runtime functions are designed a specific way based on the OMP standard. If that's not the case, then it doesn't matter. I'll drop this change as well. There is a handful of hints anyway. ================ Comment at: llvm/lib/Frontend/OpenMP/OMPConstants.cpp:122 + } +} + ---------------- jdoerfert wrote: > fghanim wrote: > > 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. > Let's not add stuff that might be used. I just looked this up. Apparently, Fortran doesn't seem to have the concept of void pointer natively. However, They have something called `Assumed types` , which was added for interoperability with C, which *probably* means it will follow whatever C does to handle void pointers. I'll remove it now, and if they choose, people can add it later. ================ Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:106 + initializeVoidPtrTy(VoidPtrTy); +} + ---------------- jdoerfert wrote: > fghanim wrote: > > jdoerfert wrote: > > > I guess we can directly call the initialize functions, right? > > > With an assert that SizeTy is set we can make sure users do it ;) > > Sure, then again, I am just following in the stylistic footsteps of the > > pioneers who started the OMPBuilder, who created an initialize for > > initialize ;) > > > > On second thought, I think I will just create something like > > `initializeTargetSpecificTypes()` sort of thing, and be done with it. > > instead of having one for each type. > Point taken, though I'm fine with criticizing my own design :) > > My suggestion now: merge everything into the `OpenMPIRBuilder::initialize` we > already have. I'm also fine with removing this and letting users call (a > single) `initializeTypes`, thought it is a bit harder to "know" that is > needed. > > I think either way is fine, so I wasn't criticizing. I was explaining why I did what I did. :) Anyways, I made the change. 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
