jdoerfert added a comment. In D79675#2037484 <https://reviews.llvm.org/D79675#2037484>, @fghanim wrote:
> In D79675#2037314 <https://reviews.llvm.org/D79675#2037314>, @jdoerfert wrote: > > > I left some comments on the type stuff. The rest looks good. > > I think if you rebase the type stuff on D79739 > > <https://reviews.llvm.org/D79739> (which I can merge) we should only need > > to expand `initializeTypes` to make this work as expected. WDYT? > > > Currently, I implemented the changes relevant to me from that and made a > separate local commit prior to this one to make sure things work. > I build llvm locally, and so it take 6 - 8 hours, so when all patches are > accepted, I'll do a rebase, etc. in one go to make sure there are no problems. What takes 6-8h? Are you sure you don't want to merge parts that went through review as soon as possible? I mean, at least the type parts would be helpful. ================ 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: > > 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. ================ Comment at: llvm/lib/Frontend/OpenMP/OMPConstants.cpp:122 + } +} + ---------------- 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. ================ Comment at: llvm/lib/Frontend/OpenMP/OMPConstants.cpp:69 void llvm::omp::types::initializeTypes(Module &M) { if (Void) ---------------- fghanim wrote: > jdoerfert wrote: > > Maybe we can/should take the IntPtr and SizeTy here so there is only a > > single call necessary that initializes all types. I fear "default" values > > are problematic. > > > > WDYT? > you mean pass it as a second argument? Sure, I would love that. I mean it's > fine with me either way. I originally wanted to do that. but then thought > maybe it is slightly better to explicitly initialize target specific data > types, to give initialize type a cleaner look ... maybe?! Yes, second (and third) argument. Given that every user has to initialize all the types we can just make it a single call. ================ Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:106 + initializeVoidPtrTy(VoidPtrTy); +} + ---------------- 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. 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
