[PATCH] D54504: [stack-safety] Local analysis implementation

[Vlad Tsyrklevich via Phabricator via llvm-commits]

vlad.tsyrklevich added inline comments.


================
Comment at: llvm/lib/Analysis/StackSafetyAnalysis.cpp:119
+
+uint64_t getStaticAllocaAllocationSize(const AllocaInst *AI) {
+  const DataLayout &DL = AI->getModule()->getDataLayout();
----------------
Would be worth copying over the comment from the SafeStack pass:
  /// Calculate the allocation size of a given alloca. Returns 0 if the
  /// size can not be statically determined.


================
Comment at: llvm/lib/Analysis/StackSafetyAnalysis.cpp:133
+
+// FunctionStackSummary could also describe return value as depending on one or
+// more of its arguments.
----------------
Mark this a TODO and update the struct name.


================
Comment at: llvm/lib/Analysis/StackSafetyAnalysis.cpp:135
+// more of its arguments.
+struct llvm::StackSafetyFunctionInfo {
+  // May be a Function or a GlobalAlias
----------------
This uses a different naming convention than the structs above though it's part of the same 'type set' as the *Info structs above which is confusing. Couldn't immediately think of a fix that makes it nicer though. Might just be worth a comment to at least be explicit about why this is.


================
Comment at: llvm/lib/Analysis/StackSafetyAnalysis.cpp:321
+        }
+        // Don't visit function return value: if it depends on the alloca, then
+        // argument range would be full-set.
----------------
Is this comment outdated? I can't figure out what it's referring to, Maybe @eugenis knows.


Repository:
  rL LLVM

https://reviews.llvm.org/D54504