[PATCH] D21645: [CFLAA] Propagate StratifiedAttrs from callee to caller

[George Burgess IV via llvm-commits]

george.burgess.iv added a comment.

Thanks for the patch!


================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:94
@@ +93,3 @@
+/// We use ExternalAttribute to describe an externally visible StratifiedAttrs
+/// for parameters/return value
+struct ExternalAttribute {
----------------
Nit: Please add a period.

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:107
@@ -99,1 +106,3 @@
 
+  // RetParamAttributes is a collection of ExternalAttributes
+  SmallVector<ExternalAttribute, 8> RetParamAttributes;
----------------
Nit: Period, please.

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:151
@@ +150,3 @@
+LLVM_CONSTEXPR StratifiedAttr AttrCaller = 1 << AttrCallerIndex;
+LLVM_CONSTEXPR StratifiedAttr ExternalAttrMask =
+    AttrEscaped | AttrUnknown | AttrGlobal;
----------------
Is there a reason this isn't a `StratifiedAttrs`? (The more we add here, the more I think having a separate type for a single bit and a set of bits was a dumb decision, but we can deal with that later)

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:267
@@ -266,3 @@
-  struct Node {
-    Value *Value;
-    unsigned DerefLevel;
----------------
Please rebase -- GCC didn't like the name of this variable. :)

(Don't worry about compiling with GCC before you throw a patch up for review; I'm happy to do it prior to submitting)

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:645
@@ -602,3 +644,3 @@
 
-    GetEdgesVisitor(Analysis, TLI, Graph, ExternalValues, EscapedValues,
-                    InterprocEdges)
+    GetEdgesVisitor(Analysis, TLI, Graph, ReturnedValues, ExternalValues,
+                    EscapedValues, InterprocEdges, InterprocAttrs)
----------------
Nit: Is there a reason this isn't a member of the CFLGraphBuilder?

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:736
@@ -690,3 +735,3 @@
 static bool isGlobalOrArgAttr(StratifiedAttrs Attr) {
   return Attr.reset(AttrEscapedIndex).reset(AttrUnknownIndex).any();
 }
----------------
Do we need a `.reset(AttrCallerIndex)` here, as well?

================
Comment at: lib/Analysis/CFLAliasAnalysis.cpp:828
@@ +827,3 @@
+      auto ExternalAttrs = Link.Attrs & StratifiedAttrs(ExternalAttrMask);
+      if (ExternalAttrs.any()) {
+        RetParamAttributes.push_back(
----------------
Please remove the extra brackets

================
Comment at: test/Analysis/CFLAliasAnalysis/interproc-store-arg-unknown.ll:20
@@ -22,3 +19,3 @@
 ; CHECK: NoAlias: i32* %b, i32* %lp
-; CHECK: MayAlias: 32* %lp, i32** %p
+; CHECK: NoAlias: i32* %lp, i32** %p
 define void @test_store_arg_unknown(i32* %x) {
----------------
Yay accuracy


http://reviews.llvm.org/D21645