[PATCH] D57627: [BasicAA] Cache nonEscapingLocalObjects for alias() calls.

[Alina Sbirlea via Phabricator via llvm-commits]

asbirlea added a comment.

Also wondered if the duplicated 4 lines are worth adding a lambda. Didn't seem worth it, but if you feel differently, I'll be happy to update it.



================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:122-126
+  if (IsCapturedCache) {
+    auto CacheIt = IsCapturedCache->find(V);
+    if (CacheIt != IsCapturedCache->end())
+      return CacheIt->second;
+  }
----------------
chandlerc wrote:
> Can this function ever be re-entered? It seems unlikely given the structure.
> 
> If not, hoist the `CacheIt` iterator out, use insert, and then update it below rather than doing another lookup?
IIUC, insert doesn't do a lookup. So using insert below (instead of `[]`) should be equivalent to your suggestion? We are guaranteed we need to insert if we reached that point..


================
Comment at: lib/Analysis/BasicAliasAnalysis.cpp:154-155
+    }
 
   return false;
 }
----------------
chandlerc wrote:
> Why not cache this too?
I debated between sinking the cache search into each of the above `if`s, or caching this.

Performance-wise, I see no difference between the cache search on all queries vs the checks inside the above `if`s. So, sure, let's cache this too :)

If other folks find that querying the cache is more expensive when all conditions (`isa`+`isNoAliasCall`+ `dyn_cast`+attributes check) are `false`, then I can refactor this.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57627