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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert marked 2 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:
> 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`).


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

```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69930