[PATCH] D23432: [AliasSetTracker] Degrade AliasSetTracker results when may-alias sets get too large.

[Michael Kuperstein via llvm-commits]

mkuper marked 5 inline comments as done.
mkuper added a comment.

Thanks, David!


================
Comment at: include/llvm/Analysis/AliasSetTracker.h:132-133
@@ -127,2 +131,4 @@
+  // Use when the tracker holding this set is saturated.
+  unsigned AlwaysAlias : 1;
 
   /// The kinds of access this alias set models.
----------------
davidxl wrote:
> How about it be called  'AliasAny' ?
Sounds good.

================
Comment at: include/llvm/Analysis/AliasSetTracker.h:418
@@ +417,3 @@
+  // all pointers into a single "May" set.
+  AliasSet *SaturatedAS;
+
----------------
davidxl wrote:
> nit: AliasAnyAS may sound better
Sure, wasn't happy with the "saturated" name to begin with.
Do you have any name suggestions for the option, too? (after this change, that's the only place "saturation" will appear in, which may be a bit confusing).

================
Comment at: lib/Analysis/AliasSetTracker.cpp:29
@@ -28,1 +28,3 @@
 
+static cl::opt<unsigned> SaturationThreshold("alias-set-saturation-threshold",
+                                             cl::Hidden, cl::init(250));
----------------
davidxl wrote:
> add option description.
Ugh, right, thanks!

================
Comment at: lib/Analysis/AliasSetTracker.cpp:294
@@ +293,3 @@
+  if (SaturatedAS) {
+    // Update the data structure, for the sake of consistency.
+    // Note that we will never need a merge here, since there's only one
----------------
davidxl wrote:
> can this logic of checking SaturatedAS be folded into 'mergeAliasSetsForPointer such that there is no need to make any change in this method.
It can, but since this is a major behavior difference between a saturated and a non-saturated AST, I think it would be clearer to leave it here.

mergeAliasSetsForPointer used to be called findAliasSetsForPointer. I changed the name in r266381 - the idea was to more clearly separate between the "merging" behavior, and the "get/find the right set" behavior. See discussion in https://reviews.llvm.org/D18939?id=53163#inline-159392

Do you see a specific reason for this to live in merge()?

================
Comment at: lib/Analysis/AliasSetTracker.cpp:554
@@ +553,3 @@
+    // Otherwise, we need to do the actual work of copying.
+    if (!Cur->UnknownInsts.empty()) {
+      if (SaturatedAS->UnknownInsts.empty())
----------------
davidxl wrote:
> Use SaturatedAS->MergeSetIn(Cur ...) ?
Right, was going to change this between the two versions, and it slipped my mind. I think I can't use MergeSetIn() as is, but I can factor out the common logic.


https://reviews.llvm.org/D23432