[PATCH] D118669: [Attributor] Introduce the concept of query AAs

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1504
           ChangedAAs.push_back(AA);
+        else if (AA->isQueryAA())
+          QueryAAs.push_back(AA);
----------------
kuter wrote:
> NIT: This is not going to add the AA to `QueryAAs` list if a query AA returns `UNCHANGED`.
> The name `QueryAAs` seems confusing. 
> 
> 
Confusing how?

This should ensure a query AA is either in the ChangedAAs container, or in the QueryAAs container, not both.
What am I missing?


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1522
     Worklist.clear();
+    Worklist.insert(QueryAAs.begin(), QueryAAs.end());
     Worklist.insert(ChangedAAs.begin(), ChangedAAs.end());
----------------
kuter wrote:
> So we effectively update every QueryAA ever created, every iteration right ? I don't that's needed.
> We don't need to update a query aa if it doesn't have any unresolved queries but we need to update it when it receives a new query, more on this below.
That is a fair point. I'll rewrite it and only update the ones with new queries.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1529
+                    !llvm::any_of(QueryAAs, [](AbstractAttribute *QueryAA) {
+                      return QueryAA->hasNewQueries();
+                    });
----------------
kuter wrote:
> I think it would be better if AA indicated to the attributor that it received new queries rather than the attributor asking them if they received any. 
> 
> We always take an attributor instance in the query functions, why don't we have a function in `Attributor` that a query AA can call and ask for an update ?
> 
Because the fixpoint iteration state is not part of the object, at least not right now. All lives inside of `run`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118669