[PATCH] D69930: [OpenMP] Introduce the OpenMPOpt transformation pass

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert marked 4 inline comments as done.
jdoerfert added inline comments.


================
Comment at: llvm/include/llvm/Transforms/IPO/OpenMPOpt.h:19
+/// OpenMP optimizations pass.
+struct OpenMPOptPass : public PassInfoMixin<OpenMPOptPass> {
+  PreservedAnalyses run(LazyCallGraph::SCC &C, CGSCCAnalysisManager &AM,
----------------
ABataev wrote:
> jdoerfert wrote:
> > ABataev wrote:
> > > Better to use `class`? Also, maybe worth it to mark it as `final`.
> > I can do that but I do not understand why this would be better (in a lot of situations, including this one).
> > After all, class will just cause an additional `public` and that is it (for all but MSVC and only if we declare this struct/class again which we never do). Similarly, I have never seen an attempt to inherit from a new-PM style pass and if so it is unclear why we should forbid it (with `final`).
> Up to you. But if you going to have `private` or `protected` members later, better to make `class` rather than `struct`.
> Up to you. But if you going to have private or protected members later, better to make class rather than struct.

Why? They are the same except the default visibility.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:145
+        if (CallInst *CI = getCallIfRegularCall(*U, &RFI)) {
+          CI->moveBefore(&*F.getEntryBlock().getFirstInsertionPt());
+          ReplVal = CI;
----------------
ABataev wrote:
> jdoerfert wrote:
> > ABataev wrote:
> > > What if the function is already in the entry block?
> > Not a problem and not sufficient:
> > 
> > If we have the following entry and the first use we visit is (for some reason) `%gtid1` we need to move it because it would not dominate the use of `%gtid0`.
> > ```
> > entry:
> >   %gtid0 = call __kmpc_global_thread_num ...
> >   ... use(%gtid0)
> >   %gtid1 = call __kmpc_global_thread_num ...
> > 
> > ```
> The question is a little bit different. What if we have something simple:
> ```
> entry:
>   %gtid0 = call __kmpc_global_thread_num ...
>   ... use(%gtid0)
> ```
> Will this pass try to move the call after the use? I'm not saying that this will definitely happen, I'm just asking if pass of aware of such situation and will handle it properly.
This moves the call unconditionally to the very first position in a function. That is by definition before all uses. We can add smarts wrt. the placement later.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69930