[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:
> > 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. 

================
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:
> > 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?

================
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) {
----------------
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.

================
Comment at: lib/Analysis/AliasAnalysis.cpp:171
@@ +170,3 @@
+  // modify the memory location.
+  if ((Result & MRI_Mod) &&
+      pointsToConstantMemory(Loc, /*OrLocal*/ false))
----------------
chandlerc wrote:
> Gerolf wrote:
> > Why could Mod be set in this case? Wouldn't that be a bug?
> If this is a bug, it is a bug in existing code, so you should be able to find a test case and fix it separately from this patch?
Why does the code check for this condition rather asserting? I'm wondering how the code above could diagnose a Mod for a constant location.


http://reviews.llvm.org/D17329