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

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a comment.

In D72304#1815793 <https://reviews.llvm.org/D72304#1815793>, @fghanim wrote:

> > 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?


Already for this one, at least the git commit.
You can edit this patch in phab (top right, edit revision) and you need to make sure to adjust the git commit before pushing.

>> 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?

We can use lit test for that. The two files that you need to modify for sure are `clang/test/OpenMP/critical_codegen.cpp` and `clang/test/OpenMP/master_codegen.cpp`. You add run lines so that the code in clang is actually executed and modify the check lines appropriately.
I did split my commits into the llvm and clang part mostly, if you look at a clang patch which uses the OpenMPIRBuilder, you should see what I mean.

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

I did respond to the ones with.



================
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:3020
+      if (AllocaIP.isSet())
+        AllocaInsertPt = &*AllocaIP.getPoint();
+      auto OldReturnBlock = ReturnBlock;
----------------
fghanim wrote:
> 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.
I try not to make assumptions like "Inlined directives should emit their allocas to the entry block of the containing outined function, right" if we don't need to. If we do, the assert is probably a better way to go. I would actually say we want a helper that sets/resets all these things for *all* body functions. If the OMPBuilder doesn't want to set a new allocaip, it should provide an "invalid" location to indicate that. Now I finally see what you do here and why. Go with the assert for now, and make it `!isSet()` if there is no reason to complicate it further.


================
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:3076
+      EmitBranchThroughCleanup(Dest);
+    };
+
----------------
fghanim wrote:
> 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.
Agreed. We need a helper that is used at both/all places.


================
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:3099
+      ReturnBlock = OldReturnBlock;
+    };
+
----------------
fghanim wrote:
> jdoerfert wrote:
> > Same here, that looks like the other "BodyGenCB". We should really make them helpers. 
> Same as prev.
Again a TODO and we are good for now.


================
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
----------------
fghanim wrote:
> 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.
I take it you'll remove them. Just saying because you commented ;)


================
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 &&
----------------
fghanim wrote:
> jdoerfert wrote:
> > Nit: `InsertPointTy FiniIP(FiniBB, FiniBB->getFirstInsertionPt());`
> Nit?
Nit is often used to indicate a request that can be ignored silently (https://www.urbandictionary.com/define.php?term=nit)


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


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:808
+                                     StringRef FirstSeparator,
+                                     StringRef Separator) const {
+  SmallString<128> Buffer;
----------------
fghanim wrote:
> 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.
You can keep it a separate helper but I still don't see how it helps to keep this a member (instead of a static).

I also want the name change. My rational is simple: If we pick a name that is meaningful, people know what it does regardless of whether they know the version in clang.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:824
+  Out << Name;
+  StringRef RuntimeName = Out.str();
+  auto &Elem = *InternalVars.try_emplace(RuntimeName, nullptr).first;
----------------
fghanim wrote:
> 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.
Here, the argument that people recognize it doesn't work at all, it's a type. So please pass a StringRef if there is no other reason not to.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:836
+             /*InsertBefore=*/nullptr, llvm::GlobalValue::NotThreadLocal,
+             AddressSpace);
+}
----------------
fghanim wrote:
> 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.
The code pattern is unrelated from "same as prev", I hope.

For the linkage, add a TODO to investigate if private linkage would work as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72304