[PATCH] D72304: [OpenMP]{OMPIRBuilder] Add Directives (master and critical) to OMPBuilder.

[Fady Ghanim via Phabricator via llvm-commits]

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

I am working on a patch. In the mean time ;)



================
Comment at: clang/lib/CodeGen/CGStmtOpenMP.cpp:3020
+      if (AllocaIP.isSet())
+        AllocaInsertPt = &*AllocaIP.getPoint();
+      auto OldReturnBlock = ReturnBlock;
----------------
jdoerfert wrote:
> 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.
I think most (if not all) llvm passes that are concerned with allocas, expect them to be in the entry block, this is why I made this assumption. The only time that I can think of, where this will not be true is in case of a delayed outlining. In that case, the insertion block is going to be the "future" entry block of the "future" outlined function, and needs to be passed. Then I think it is also necessary to keep track of the insertion block somewhere inside the OMPBuilder.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:800
+  ExitCall->removeFromParent();
+  Builder.Insert(ExitCall);
+
----------------
jdoerfert wrote:
> `ExitCall->moveBefore(IP)`, potentially with some IP = ... above.
> We should specify what the builder insert position is after this call, maybe "unspecified"?
before this point, the builder is always set to insert before the terminator of the finalization block. when `emitInlinedFunction` is done, it sets it to `ExitBB.end()`


================
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:
> 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.
You are right. *That* argument doesn't work. However the argument that I found it this way in clang kinda does. Also the argument that it makes things much easier to move. ;)
I assume whoever did it this way had his/her good reason. I understand you may not think it is, but given that it's used a lot in clang for getting and creating internal OMP variables, you can't really judge without going over every case to make sure that what you want can be done in every case, as such I am against the change *NOW*. However, for the time being, I included a todo basically saying that.


================
Comment at: llvm/lib/Frontend/OpenMP/OMPIRBuilder.cpp:836
+             /*InsertBefore=*/nullptr, llvm::GlobalValue::NotThreadLocal,
+             AddressSpace);
+}
----------------
jdoerfert wrote:
> 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.
The assert is only needed if `Elem.second` already exist. It's quite useless if it's not and we are creating it, right?
I'll change it to an `if..else`, and return it once.

critical name -which is what this is being used for now- has an external linkage, isn't it?
Also, as I said this is being used by many others all over CG_OMP to create OMP internal variables.
Maybe later we'll create different versions for different internal variables, but for now I'll add a todo anyway.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72304