[PATCH] D59918: [Attributor] Pass infrastructure and fixpoint framework

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert marked 5 inline comments as done.
jdoerfert added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:78
+/// Helper to adjust the statistics.
+static void bookkeeping(AbstractAttribute::ManifestPosition MP,
+                        const Attribute &Attr) {
----------------
fhahn wrote:
> This doesn't do anything at the moment?
It has a call site but since there are no attributes yet, nothing is done.
I wanted to keep it in this patch because it is part of the framework. Though, I expected this patch to land only with at least one attribute.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:319
+    for (AbstractAttribute *AA : Worklist)
+      if (AA->update(*this) == ChangeStatus::CHANGED)
+        ChangedAAs.push_back(AA);
----------------
jdoerfert wrote:
> fhahn wrote:
> > Passing in the whole Attributor seems a bit heavy handed. 
> > 
> > Conceptually, IIUC, to update an attribute, we just need a way to query other attributes (and potentially some other IR info) as input from the attributor, right?
> Correct, query other in-flight attributes and cached IR information.
> 
> I pass the Attributor because the above functionality is pretty much everything it exposes as public interface anyway (I think). Would you like to have another thin interface layer on top?
> 
I slimmed down the Attributor further. What do you think now?


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:451
+
+  for (Instruction &I : instructions(&F)) {
+    bool IsInterestingOpcode = false;
----------------
jdoerfert wrote:
> fhahn wrote:
> > Is the plan here for the Attributor to provide an interface for the Attribute implementations to query "interesting" instructions, instead of the Attribute implementations looking at the IR themselves? It seems this might impose a tighter coupling of Attributes <-> Attributor than intended with the flexible attribute registration system.
> > 
> > Related to the comment at line 319, it might be worth splitting this into a more restricted query/info interface that Attribute::update can use. It seems not related to the fixed point iteration in Attributor.
> The first part goes into a very interesting direction I have thought about as well. Without doing performance tests (mostly because I didn't have any attributes etc.) I choose this solution for "performance" reasons. If we invert it, which we still can and which I do not oppose, every attribute is queried for every instruction in order to decide if it is interesting to cache it, at least in the naive implementation I have in mind right now.
> 
> If we can come up with a good interface to expose IR information, probably cached, that would be great. If we put that, and the attribute query interface outside the Attributor, that would be OK. Though, the attributor will need to know about attribute queries (now through `Attributor::getAAFor<...>(...)` to track dependencies).
> 
> So far I decided this coupling is not too bad even though we have to hardcode "interesting" here. If no one has super strong feelings about this, I'd argue we go with this for now and once we have a good handle on the use cases we try to design a clean interface. But again, I will not oppose any proposal for such an interface right now.
The new separation of `InformationCache` allows you to extend it for custom attributes. I kept the old way for "default" attributes for now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59918