[PATCH] D72304: Summary: Add OpenMP Directives (master and critical) to OMPBuilder, and use them in clang.

[Fady Ghanim via Phabricator via llvm-commits]

fghanim marked 10 inline comments as done.
fghanim added a comment.

> Commits need a message that explains the change.
>  Also the "Summary" should not be in the commit title.

Is this something for future commits or do you want me to change these?
If the latter, how do I do that?

> This adds support for clang to build the directives via the OMPIRBuilder so we need to add tests for that. Probably "just" running existing codegen tests for the directives with the OMPIRBuidlder enabled.

You mean unit tests? or something else? If not unit tests, Where do I do that?

As for rest of comments, anything without response, is something I will do.



================
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:3020
+      if (AllocaIP.isSet())
+        AllocaInsertPt = &*AllocaIP.getPoint();
+      auto OldReturnBlock = ReturnBlock;
----------------
jdoerfert wrote:
> Why the `isSet` check? If we need it, then it's missing in other places as well (I think).
While I was writing the response I realized that I should've made it
` assert((!AllocaIP.isSet() || AllocaInsertPt == AllocaIP.getpoint()) && "Inlined directives allocas should be emitted to entry block of the inner most containing outlined function"; ` 
or something less verbose.

Inlined directives should emit their `alloca`s to the entry block of the containing outined function, right? For that reason, and since we don't store an `alloca` insertion point in the OMPBuilder, I pass an empty insertion point, and back up the current point, without changing it.
I was planning, in the future, to keep a copy of the most recent alloca insertion point which we set when we outline, and pass that for `BodyCB`s within inlined directives.


================
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:3060
+            ? nullptr
+            : Builder.CreateIntCast(EmitScalarExpr(Hint), CGM.Int32Ty, false);
+
----------------
jdoerfert wrote:
> Can we do
> ```
> llvm::Value *HintInst = nullptr;
> if (Hint)
>   ...
> ```
> these three lines look ugly :(
np.

btw, the todo note is about the `CGM.Int32Ty`. Currently in clang it's `CGM.IntTy`. I'll expand on the todo


================
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:3076
+      EmitBranchThroughCleanup(Dest);
+    };
+
----------------
jdoerfert wrote:
> I somehow have the feeling we could have a "FiniCB" helper function as they seem to be always the same. A TODO mentioning that is also OK for now.
I'll do a Todo for now. I am using a very similar `FiniCB` to the one you used for `parallel`. So it doesn't make sense to change these here and not that one with them.


================
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:3099
+      ReturnBlock = OldReturnBlock;
+    };
+
----------------
jdoerfert wrote:
> Same here, that looks like the other "BodyGenCB". We should really make them helpers. 
Same as prev.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPConstants.cpp:68
+  //  		  	  Type::getInt32Ty(M.getContext()),/*NumElements*/ 8);
+  //  KmpCriticalNamePtrTy = PointerType::getUnqual(KmpCriticalNameTy);
   // Create all simple and struct types exposed by the runtime and remember
----------------
jdoerfert wrote:
> These comments that reference `KmpCriticalNameTy` do not help here. If you want, feel free to add comments that explain how the result looks in a generic way but only showing `KmpCriticalNameTy` seems counterproductive to me.
I was testing something, and I forgot to remove them when I was done.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:717
+  // emit exit call and do any needed finalization.
+  auto FinIP = InsertPointTy(FiniBB, FiniBB->getFirstInsertionPt());
+  assert(FiniBB->getTerminator()->getNumSuccessors() == 1 &&
----------------
jdoerfert wrote:
> Nit: `InsertPointTy FiniIP(FiniBB, FiniBB->getFirstInsertionPt());`
Nit?


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:721
+         "Unexpected insertion point for finalization call!");
+  emitCommonDirectiveExit(OMPD, FinIP, ExitBB, ExitCall, /*hasFinalize*/ true);
+
----------------
jdoerfert wrote:
> Shouldn't we use the argument here?
"The argument" refers to?


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:808
+                                     StringRef FirstSeparator,
+                                     StringRef Separator) const {
+  SmallString<128> Buffer;
----------------
jdoerfert wrote:
> This can be a static helper and the name should be more descriptive, `getName` is very generic.
This is taken from clang (I think CG_OMP)  as is. I am keeping everything as is for now in case somebody decided to help and came across it being used somewhere else, they should recognize it immediately.

So for now, I suggest a todo. cos if I find that it's not used except with `critical`, I'll merge all 3 functions into 1 and be done with it.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:824
+  Out << Name;
+  StringRef RuntimeName = Out.str();
+  auto &Elem = *InternalVars.try_emplace(RuntimeName, nullptr).first;
----------------
jdoerfert wrote:
> This seems to be too much trouble to get a StringRef from a Twine. Maybe start with a StringRef and avoid all of it?
Same as prev.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:836
+             /*InsertBefore=*/nullptr, llvm::GlobalValue::NotThreadLocal,
+             AddressSpace);
+}
----------------
jdoerfert wrote:
> Do we really want common linkage for "internal variables"? Internal or private would have been my first guess. Maybe the "internal" part is different than I expect it.
> 
> ---
> 
> FWIW, the usual pattern is
> ```
> if (!Elem.second)
>   Elem.second = ...
> assert(...)
> return Elem.second;
> ```
same as prev.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72304