[PATCH] D17329: [AA] Hoist the logic to reformulate various AA queries in terms of other parts of the AA interface out of the base class of every single AA result object.

[Gerolf Hoflehner via llvm-commits]

Gerolf added inline comments.

================
Comment at: lib/Analysis/AliasAnalysis.cpp:139
@@ +138,3 @@
+  // aggregate set of AA results.
+  auto MRB = getModRefBehavior(CS);
+  if (MRB == FMRB_DoesNotAccessMemory)
----------------
chandlerc wrote:
> Gerolf wrote:
> > chandlerc wrote:
> > > Gerolf wrote:
> > > > This routine queries AA again but this time for behavior rather than ModRefInfo, but the result (no mod ref info) could be the same. Can this be combined in some meta routine so that no mod ref is spit out early? The AA loops can be very compile-time expensive so we should avoid/shorten them when possible.
> > > This is interesting, but seems really orthogonal to this change. I don't want to try to fix everything in one go, I'm trying to fix a specific problem with the existing implementation.
> > My concern here is compile-time. The AA loops are compile-time intense and this code could make it worse. For a change like this it would be great if you could collect compile-time data before you commit. 
> I'm confused...
> 
> As far as I can tell, this change *cannot* make the compile time worse. Previously, this exact code existed in *every* base class for an AAResults, and would have been reached by the loop above for each one of them. That means it would have been run at least once (via BasicAA) and in many cases N times for the number of AAs active. In common Clang compiles that would be at least 4 times. So we're running the same code 1/4th as often with my change.
> 
> Unless I've missed something?
Hm, now I"m confused, although I can now see your ct argument.

Correct me wrong: The new code hoists the loop over the AA's hidden in AA->getModRef(CS, Loc) out of the loop. This makes the code more efficient. But isn't that function now just returning MRI_ModRef which renders loop above dead? 


================
Comment at: lib/Analysis/AliasAnalysis.cpp:149
@@ +148,3 @@
+    ModRefInfo AllArgsMask = MRI_NoModRef;
+    if (doesAccessArgPointees(MRB)) {
+      for (auto AI = CS.arg_begin(), AE = CS.arg_end(); AI != AE; ++AI) {
----------------
chandlerc wrote:
> Gerolf wrote:
> > Gerolf wrote:
> > > chandlerc wrote:
> > > > Gerolf wrote:
> > > > > Could you refactor into a function?
> > > > This is directly moving code from one place to another here. I don't really want to refactor all of it to look different at the same time. This applies to most of the remaining comments as well -- all of this code existed pretty much exactly as-is before. But if there is a desire to refactor this, I think that should be a separate patch.
> > > My concern is compile-time. The AA loops are compile-time intense and I suspect this code makes it worse. Do your data show that is not the case?
> > Makes sense. Also digging a bit more into the code this is not as straightforward as I thought it was.
> See above for why this too cannot (AFAICT) be making the compile time worse.
Sure, but I was not concerned about ct here anyway. No issue. Thanks for your clarifications!


http://reviews.llvm.org/D17329