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

[Stefan Stipanovic via Phabricator via llvm-commits]

sstefan1 marked 7 inline comments as done.
sstefan1 added a comment.

In D81788#2091453 <https://reviews.llvm.org/D81788#2091453>, @jdoerfert wrote:

> 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).


Maybe D81114 <https://reviews.llvm.org/D81114> should be commited first, or should I just abandon that and put the tests here?

I'll split this with at least two more patches.



================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:822
         InfoCache(InfoCache), CGUpdater(CGUpdater), Whitelist(Whitelist) {}
 
   ~Attributor();
----------------
jdoerfert wrote:
> Why do we want a pointer here? We can derive from `InformationCache` elsewhere and still provide a reference, right?
So that we can later cast it to `OMPInformationCache` and use openmp-specific information. This seemed like least amount of change.


================
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
----------------
jdoerfert wrote:
> 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.
I guess then it should also be allowed to be used after Attributor run is finished? Say we want to know ICV value after the Attributor run and all the deduplication, we still won't have querying attribute. Or should we also expose `lookupAAFor`?


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:929
+  }
+
   /// Explicitly record a dependence from \p FromAA to \p ToAA, that is if
----------------
jdoerfert wrote:
> 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.
You mean a NFC commit without a review?


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:2980
+// };
+
 /// Run options, used by the pass manager.
----------------
jdoerfert wrote:
> 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? 
I think this should work, but I'll have to put more thought into (mainly the registration mechanism). this I'm in favor of getting this in first,  since this is already proven to be rather large patch with lots of changes. 


================
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) {
----------------
jdoerfert wrote:
> Documentation gone? We need some for both functions.
Forgot to copy. Will do.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:127
       if (!UV)
-        UV = std::make_unique<UseVector>();
+        UV = std::make_shared<UseVector>();
       return *UV;
----------------
jdoerfert wrote:
> Why this change?
I think this comes from the fact that the same `OMPInfoCache` is shared between the Attributor and OpenMPOPT. Maybe we'll be able to avoid this after splitting up the patch, but I'm not sure.


================
Comment at: llvm/lib/Transforms/IPO/OpenMPOpt.cpp:815
+        if (pair.first->getParent() == Pred)
+          return pair.second;
+
----------------
jdoerfert wrote:
> 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
I agree. Not to proud of this part tbh. I'll leave this out for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81788