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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

I'm really not the best person to review this :/
It seems generally within what'd expect, but i'm not really familiar with SCC passes.
Some thoughts:



================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:35
+
+static constexpr auto TAG = "[OpenMPOpt] ";
+
----------------
Traditionally `DEBUG_TYPE` itself is used for this.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:116
+      Value *GTIdArg = nullptr;
+      llvm::any_of(F->args(), [&](Argument &Arg) {
+        return GTIdArg = GTIdArgs.count(&Arg) ? &Arg : nullptr;
----------------
I'm guessing this is 'llvm::for_each() but until first match'?
Maybe something like 

Value *GTIdArg = find_if() be better?


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:214-215
+    // Transitively search for more arguments by looking at the users of the
+    // ones we know already.
+    for (unsigned u = 0; u < GTIdArgs.size(); ++u)
+      AddUserArgs(*GTIdArgs[u]);
----------------
Here and elsewhere: i'm guessing `AddUserArgs()` will update `GTIdArgs` so it must not be range-based for and we can't precache `GTIdArgs.size()`. This should be commented.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69930