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

[Kuter Dinel via Phabricator via llvm-commits]

kuter added inline comments.


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




================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1522
     Worklist.clear();
+    Worklist.insert(QueryAAs.begin(), QueryAAs.end());
     Worklist.insert(ChangedAAs.begin(), ChangedAAs.end());
----------------
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.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1529
+                    !llvm::any_of(QueryAAs, [](AbstractAttribute *QueryAA) {
+                      return QueryAA->hasNewQueries();
+                    });
----------------
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 ?



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118669