This is an archive of the discontinued LLVM Phabricator instance.

[Attributor] Introduce the concept of query AAs
ClosedPublic

Authored by jdoerfert on Jan 31 2022, 5:11 PM.

Details

Summary

D106720 introduced features that did not work properly as we could add
new queries after a fixpoint was reached and which could not be answered
by the information gathered up to the fixpoint alone.

As an alternative to D110078, which forced eager computation where we
want to continue to be lazy, this patch fixes the problem.

QueryAAs are AAs that allow lazy queries during their lifetime. They are
never fixed if they have no outstanding dependences and always run as
part of the updates in an iteration. To determine if we are done, all
query AAs are asked if they received new queries, if not, we only need
to consider updated AAs, as before. If new queries are present we go for
another iteration.

Diff Detail

Event Timeline

jdoerfert created this revision.Jan 31 2022, 5:11 PM
jdoerfert requested review of this revision.Jan 31 2022, 5:11 PM
Herald added a reviewer: baziotis. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript

I have no idea what this means, sorry.

jdoerfert updated this revision to Diff 404771.Jan 31 2022, 5:26 PM

Improved documentation/comments, added test changes

The changed tests show how we now miss norecurse in CGSCC mode, which is a good
thing. Note that this only happens we use AAFunctionReachable for AANorecurse,
thus, after D110099.

kuter added inline comments.Jan 31 2022, 5:33 PM
llvm/lib/Transforms/IPO/Attributor.cpp
1503

NIT: This is not going to add the AA to QueryAAs list if a query AA returns UNCHANGED.
The name QueryAAs seems confusing.

1518

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.

1524

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 ?

jdoerfert added inline comments.Jan 31 2022, 5:40 PM
llvm/lib/Transforms/IPO/Attributor.cpp
1503

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?

1518

That is a fair point. I'll rewrite it and only update the ones with new queries.

1524

Because the fixpoint iteration state is not part of the object, at least not right now. All lives inside of run.

kuter added inline comments.Jan 31 2022, 5:57 PM
llvm/lib/Transforms/IPO/Attributor.cpp
1503

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.

1524

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.

Address comments, use push not pull interface

kuter accepted this revision.Jan 31 2022, 10:47 PM

LGTM

This revision is now accepted and ready to land.Jan 31 2022, 10:47 PM
This revision was landed with ongoing or failed builds.Jan 31 2022, 11:43 PM
This revision was automatically updated to reflect the committed changes.