[PATCH] D81788: [WIP][OpenMPOPT] ICV Tracking Support

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

I left a first set of comments. I think the overall direction is good, some interesting opportunities are opening up (see below). We need tests and also to split the patch (some suggestions below).



================
Comment at: llvm/include/llvm/Frontend/OpenMP/OMPKinds.def:367
+
+///}
+
----------------
Remove the comment for now. and also the clause stuff.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:822
         InfoCache(InfoCache), CGUpdater(CGUpdater), Whitelist(Whitelist) {}
 
   ~Attributor();
----------------
Why do we want a pointer here? We can derive from `InformationCache` elsewhere and still provide a reference, right?


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:877
+  //         way to create AA, outside of the Attributor, without the
+  //         QueryingAA?
+  /// The private version of getAAFor that allows to omit a querying abstract
----------------
I think everything else would just duplicate code. This can be public but the comment should mention that use *after* the Attributor was started to run is restricted to the Attributor itself. Initial seeding can be done via this function.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:929
+  }
+
   /// Explicitly record a dependence from \p FromAA to \p ToAA, that is if
----------------
Just moving this to be public with a new comment LGTM. Feel free to do this right away. If you want input on the comment, let me know.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:2980
+// };
+
 /// Run options, used by the pass manager.
----------------
I think the AA should live in OpenMP opt. TBH, similar to a comment below, I would like us to have a "simplification AA" here at some point. More specifically for this use case a "call simplifier". Those interfaces would live with the Attributor and be used by it, via some registration mechanism. It might even be useful to have the general logic in here eventually so that only a few functions need to be implemented by the user, e.g., `bool interactsWithHiddenState(CallBase &CB)`, `Optional<Value> getNewHiddenState(CallBase &CB)`, `bool returnsHiddenState(CallBase &CB)`. The generic algorithm would track the "hidden state" (here ICV value) and provide all the logic. We can do that in a follow up or now. WDYT? 


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:59
+/// Attributor runs.
+/// FIXME: Maybe put this in a header file?
+struct OMPInformationCache : public InformationCache {
----------------
As long as there is no other user we keep it here.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:94
+    //
+    /// Initial Value
+
----------------
I think we need to do that eventually but for a first restricted deduction, see below, we might not need to. Just remove the comments for now then.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:117
-    /// true. The callback will be fed the function in which the use was
-    /// encountered as second argument.
     void foreachUse(function_ref<bool(Use &, Function &)> CB) {
----------------
Documentation gone? We need some for both functions.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:127
       if (!UV)
-        UV = std::make_unique<UseVector>();
+        UV = std::make_shared<UseVector>();
       return *UV;
----------------
Why this change?


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:302
+  }
+};
+
----------------
I think an RFC commit moving everything into a "cache" helper class should be provided seperatly. That seems trivial to accept and will significantly shrink this diff.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:686
+
+    // TODO: Expose runAttributorOnFunctions() or have an Attributor instance?
+
----------------
I feel the instance might be preferable so we can access AAs later, e.g., via `A.lookupAAFor`.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:688
+
+    // TODO: WhiteList some attributes? AAIsDead?
+
----------------
I think in the long run we want to register the ICV tracking attribute as another "simplification" attribute (for runtime calls). Then, the Attributor can ask all simplification attributes for a better value for a call, potentially resulting in better liveness and therefore better ICV tracking ;)

For now we could whitelist only ICV tracking (I guess).


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:808
+          // value.
+          return pair.second;
+
----------------
Style: Add braces if you have a comment that makes the body multi-line.
You have to verify there is no cal in-between the two instructions too:
```
set_num_threads(2);
foo(); // resets num_threads
get_num_threads();
```
^ negative test we should keep.

We expand our tracking efforts in a follow up, for now we can be restrictive and correct with some positive and negative tests.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:815
+        if (pair.first->getParent() == Pred)
+          return pair.second;
+
----------------
I don't think this works:

```
if (...)
  set_num_threads(2);
get_num_threads();

```
^ negative test we should keep.

For the first version go with the same basic block only


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81788