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

[Alexey Bataev via Phabricator via llvm-commits]

ABataev 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,
----------------
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`.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:145
+        if (CallInst *CI = getCallIfRegularCall(*U, &RFI)) {
+          CI->moveBefore(&*F.getEntryBlock().getFirstInsertionPt());
+          ReplVal = CI;
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69930