[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);
----------------
jdoerfert wrote:
> 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?
QueryAAs doesn't actually include all query AA's that's all.  This list just contains the ones that are inactive. The name would makes me except that it's a list of all AbstarctAttributes that are query AA's. 

I think `InactiveQueryAAs` would be a better name but this is just an nit.
Sorry if I wasn't clear enough in my first message.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1529
+                    !llvm::any_of(QueryAAs, [](AbstractAttribute *QueryAA) {
+                      return QueryAA->hasNewQueries();
+                    });
----------------
jdoerfert wrote:
> 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`.
I just think it is inefficient to go over every query AA every iteration just to get a bool flag from them.
There could be serveral query AA's per each function.

>  fixpoint iteration state is not part of the object
Yes, I know. We could have a list of AA's that explicitly asked to be updated.
We would have a function `Attributor::requestUpdate(AbstractAttribute &AA)`


This comment isn't important if you think it adds  unneeded complexity in the rest of the patch set and/or not very important performance vise. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118669