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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

I think this needs a rebase once some other landed.



================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:822
         InfoCache(InfoCache), CGUpdater(CGUpdater), Whitelist(Whitelist) {}
 
   ~Attributor();
----------------
sstefan1 wrote:
> 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.
You can cast regardless. If there is no other reason, please keep it a reference :)


================
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
----------------
sstefan1 wrote:
> 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`?
Iff you are careful, you can use this *after* a run, e.g. if you only use information from attributes at a fixpoint. Exposing lookup makes this less error prone and would allow us to have an assert even. The other way is to create and remember all interesting locations. That is usually what we did so far but if we have non AA users lookup might be useful as well.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:929
+  }
+
   /// Explicitly record a dependence from \p FromAA to \p ToAA, that is if
----------------
sstefan1 wrote:
> 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?
Yes. The code movement parts.


================
Comment at: llvm/include/llvm/Transforms/IPO/Attributor.h:2980
+// };
+
 /// Run options, used by the pass manager.
----------------
sstefan1 wrote:
> 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. 
Agreed. We can even split them further and submit the NFC parts so it becomes clearer to me what changes :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D81788