[PATCH] D64152: [Attributor] Deduce "align" attribute

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1977
 
+// ------------------------ Align Argument Attribute ------------------------
+
----------------
Are the statistics missing?


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1981
+  AAAlignImpl(Value &V, InformationCache &InfoCache)
+      : AAAlign(V, InfoCache), IntegerState(1U << 29) {}
+
----------------
Make the `1U << 29` a static const member with a proper name and description.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:1985
+              InformationCache &InfoCache)
+      : AAAlign(AssociatedVal, AnchoredValue, InfoCache) {}
+
----------------
The state is not initialized here as above. Call the general constructor from the specialized one (above) to avoid such inconsistencies.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2023
+    Attrs.emplace_back(Attribute::getWithAlignment(Ctx, getKnownAlign()));
+  }
+};
----------------
I think we should, for consistency, call `getAssumedAlign()` here.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2376
+      // Every function with pointer return type might be marked align.
+      if (ReturnType->isPointerTy() &&
+          (!Whitelist || Whitelist->count(AAAlignReturned::ID)))
----------------
remove `isPointerTy`, see above.


================
Comment at: llvm/test/Transforms/FunctionAttrs/align.ll:43
+;  ret i32* %ret
+; }
+
----------------
What happened to test5 again? Why is it commented out?


================
Comment at: llvm/test/Transforms/FunctionAttrs/align.ll:57
+  ret i32* %ret
+}
+
----------------
Please add a more meaningful SCC test, 3 functions, conditionals, malloc, etc. This one is a no-return recursion for which we can deduce everything.


================
Comment at: llvm/test/Transforms/FunctionAttrs/align.ll:75
+  tail call void @test8(i32* %ptr2, i32* %ptr1, i32* %ptr1)
+  tail call void @test8(i32* %ptr2, i32* %ptr1, i32* %ptr1)
+  ret void
----------------
check the call sites.


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

https://reviews.llvm.org/D64152