[PATCH] D79675: [OpenMP][OMPBuilder] Adding Privatization Requirements to OMPIRBuilder

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

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?



================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:336
 
-  /// Generator for '#omp master'
+  /// Generator for '#omp Critical'
   ///
----------------
Nit: unrelated (just commit it), use lower case to match OpenMP


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:387
+  CallInst *CreateOMPFree(const LocationDescription &Loc, Value *Addr,
+                          Value *Allocator, std::string name = "");
+
----------------
Style: Use `Name` for the variable name.


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:389
+
+  /// Create a runtime call for kmpc_free
+  ///
----------------
Nit: Copy and paste


================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPIRBuilder.h:400
+                                      llvm::ConstantInt *Size,
+                                      const llvm::Twine &Name = Twine(" "));
+
----------------
Do we really want a space as name here? I would understand `""` but a space seems odd to me.


================
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:
> > > > 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 ;)


================
Comment at: llvm/lib/Frontend/OpenMP/OMPConstants.cpp:69
 
 void llvm::omp::types::initializeTypes(Module &M) {
   if (Void)
----------------
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?


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:106
+  initializeVoidPtrTy(VoidPtrTy);
+}
+
----------------
I guess we can directly call the initialize functions, right?
With an assert that SizeTy is set we can make sure users do it ;)


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:942
+  //         v
+  //   OMP.Entry.Next
+
----------------
Great! thanks!


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:971
+    Builder.SetInsertPoint(br);
+  }
+
----------------
Nit: `Br` or just make it a one-liner w/o braces.


================
Comment at: llvm/unittests/Frontend/OpenMPIRBuilderTest.cpp:817
+  EXPECT_EQ(CopyinEnd,NotMasterBr->getSuccessor(0));
+}
+
----------------
Great.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79675/new/

https://reviews.llvm.org/D79675