[PATCH] D21000: [CFLAA] Cleaned up StratifiedAttrs handling

[George Burgess IV via llvm-commits]

george.burgess.iv added a comment.

Looks good after a few more comments.

> This is my mistake. I have no excuse for it :(


If no one made mistakes, code reviews wouldn't exist. Just do your best. :)

> Say I break this patch into two pieces P0 and http://reviews.llvm.org/P1, where http://reviews.llvm.org/P1 depends on P0. If for some reasons P0 get changed before it is committed in-tree, is there a clean way I can propagate the changes to http://reviews.llvm.org/P1 as well?


I use git branches. Whenever I change branch P0, I swap to http://reviews.llvm.org/P1 and rebase. It's not the prettiest solution, and sometimes requires effort, but it's the best thing I know of. If you use svn, I can't help you; my svn directory literally only exists so I can throw a patch on it and commit said patch.

> Well, I'll just leave the changes as-is because I'm too lazy to manually revert all those changes


Yeah, the refactored code seems less error-prone anyway, so I'm okay with that.


================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:116
@@ +115,3 @@
+
+bool isLocalAttr(StratifiedAttrs attr) {
+  return attr.none() || attr.test(AttrEscapedIndex);
----------------
Style nits: `attr` -> `Attr`, and we generally try to keep "static" functions `static` and out of unnamed namespaces.

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:117
@@ +116,3 @@
+bool isLocalAttr(StratifiedAttrs attr) {
+  return attr.none() || attr.test(AttrEscapedIndex);
+}
----------------
Should be `return attr.none() || attr == AttrEscaped;`?

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:1095-1114
@@ -1089,21 +1094,22 @@
 
-  // Stratified set attributes are used as markets to signify whether a member
-  // of a StratifiedSet (or a member of a set above the current set) has
-  // interacted with either arguments or globals. "Interacted with" meaning its
-  // value may be different depending on the value of an argument or global. The
-  // thought behind this is that, because arguments and globals may alias each
-  // other, if AttrsA and AttrsB have touched args/globals, we must
-  // conservatively say that they alias. However, if at least one of the sets
-  // has no values that could legally be altered by changing the value of an
-  // argument or global, then we don't have to be as conservative.
-  if (AttrsA.any() && AttrsB.any())
+  // If both values are local (meaning the corresponding set has attribute
+  // AttrNone or AttrEscaped), then we know that CFLAA fully models them and we
+  // should proceed to check set indices.
+  // If at least one value is non-local (meaning it either is global/argument or
+  // it comes from unknown sources like integer cast), the situation becomes a
+  // bit more interesting. We follow three general rules described below:
+  // - Non-local values may alias each other
+  // - AttrNone values do not alias any non-local values
+  // - AttrEscaped values do not alias globals/arguments, but they may alias
+  // AttrUnknown values
+  if (isLocalAttr(AttrsA) && isLocalAttr(AttrsB))
+    return SetA.Index == SetB.Index ? MayAlias : NoAlias;
+  if (AttrsA.none() || AttrsB.none())
+    return NoAlias;
+  if (AttrsA.test(AttrUnknownIndex) || AttrsB.test(AttrUnknownIndex))
+    return MayAlias;
+  if (AttrsA.test(AttrEscapedIndex) || AttrsB.test(AttrEscapedIndex))
+    return NoAlias;
+  else
     return MayAlias;
 }
----------------
The reason I'm fond of moving the index check up is that it obviates the need for "index check" in your table. Those all get to turn into NoAliases, and it's one less thing to worry about when reading this attribute logic. :)

Either way, I didn't think the (3) x (4) case was NoAlias when I originally commented (...though I suspect we'll need to make it MayAlias in the near future, but we'll get there when we get there), so this is a bit more complex than I originally thought. What you have now seems correct to me, so I'm happy with it.

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:1113
@@ -1100,1 +1112,3 @@
+    return NoAlias;
+  else
     return MayAlias;
----------------
Nit: No `else`.  :)


http://reviews.llvm.org/D21000