[PATCH] D94363: [AA] Pass AAResults through AAQueryInfo

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea accepted this revision.
asbirlea added a comment.
This revision is now accepted and ready to land.

In D94363#3817839 <https://reviews.llvm.org/D94363#3817839>, @nikic wrote:

> Rebase. I made an error in my previous compile-time test, it turns out that this change has no significant impact after all.
>
> The original motivation for this patch mostly went away, but I still think that doing this is a good idea. Our current AA infrastructure is really complicated, with some convoluted layering of 7 different classes (AAResults, BatchAAResults, AAQueryInfo, AAResults::Concept, AAResults::Model, AAResultBase and AAResultsProxy).
>
> This patch removes the AAResultsProxy part of the equation. An immediate followup would be to make AAResultBase non-CRTP (this patch leaves behind an unused template argument). Possible followups on top of that would be:
>
> - Make AAResultBase virtual itself and drop the AAResults::Concept and AAResults::Model indirections. I gave this a try but it turned out to be not entirely straightforward due to some NewPM interaction, but should be principally possible now.
> - Make methods that take AAQueryInfo member methods of AAQueryInfo. This should prevent accidentally not passing on AAQI. With this BatchAAResults would reduce down to AAQueryInfo, we could just make it an alias.

The current design is much better IMO than what we had before.

If you make private the AAR inside AAQI and expose methods only, then instead of calling:

  AAQI.AAR.alias(MemoryLocation::getBeforeOrAfter(*CI),
                           MemoryLocation::getBeforeOrAfter(Object), AAQI);

you'd call:

  AAQI.alias(MemoryLocation::getBeforeOrAfter(*CI),
                     MemoryLocation::getBeforeOrAfter(Object), <OptionalNewAAQI>);

I think this is what you meant with "prevent accidentally not passing on AAQI". It's not a huge cleanup though, since there still needs to be an AAQI param for the recursive phi case when a new one's needed.

-------

Is this comment outdated, or did we still have a case where the AAR could be null?

  /// A pointer to the AAResults object that this AAResult is
  /// aggregated within. May be null if not aggregated.


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

https://reviews.llvm.org/D94363