[libc-commits] [PATCH] D152226: [FunctionAttrs] Propagate some func/arg/ret attributes from caller to callsite (WIP)

[Matt Arsenault via Phabricator via libc-commits]

arsenm added a comment.

Typo guranteed in message



================
Comment at: llvm/include/llvm/Transforms/Utils/InferCallsiteAttrs.h:1
+//===- InferCallsiteAttrs.h - Propagate attributes to callsites -----------===//
+//
----------------
Missing C++ mode comment


================
Comment at: llvm/include/llvm/Transforms/Utils/InferCallsiteAttrs.h:28
+  // relatively high value is okay.
+  static constexpr unsigned kMaxChecks = UINT_MAX;
+
----------------
Does changing this to something meaningful change the compile time results?


================
Comment at: llvm/include/llvm/Transforms/Utils/InferCallsiteAttrs.h:63
+  bool checkCallerHasFnAttr(Attribute::AttrKind Attr) const {
+    return (CxtCB && CxtCB->hasFnAttr(Attr)) || Caller->hasFnAttribute(Attr);
+  };
----------------
Without looking at the rest of the context I would assume CxtB is always valid if the class is valid and all the null checks should be dropped


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:72
 
+
 STATISTIC(NumMemoryAttr, "Number of functions with improved memory attribute");
----------------
Random whitespace change


================
Comment at: llvm/lib/Transforms/IPO/FunctionAttrs.cpp:1762
+  InferCallsiteAttrs ICA;
+  for (Function *F : Nodes.SCCNodes)
+    if (F)
----------------
Braces


================
Comment at: llvm/lib/Transforms/Utils/InferCallsiteAttrs.cpp:33
+static bool addCallsiteParamAttributes(CallBase *CB,
+                                       const SmallVector<unsigned> &ArgNos,
+                                       Attribute::AttrKind Attr) {
----------------
Use ArrayRef?


================
Comment at: llvm/lib/Transforms/Utils/InferCallsiteAttrs.cpp:48
+// escape.
+static bool isCallUnkMallocLike(const Value *V) {
+  auto *MCB = dyn_cast<CallBase>(V);
----------------
Unk?


================
Comment at: llvm/lib/Transforms/Utils/InferCallsiteAttrs.cpp:53
+
+  return MCB->returnDoesNotAlias();
+}
----------------
Don't mention malloc and just say noalias call?


================
Comment at: llvm/lib/Transforms/Utils/InferCallsiteAttrs.cpp:343
+  // have derived from an argument. Finally, allocas/leaked mallocs in general
+  // are difficult (so we avoid them entirely). Callsites can arbitrarily
+  // store pointers in allocas for use later without violating a nocapture
----------------



================
Comment at: llvm/lib/Transforms/Utils/InferCallsiteAttrs.cpp:345
+  // store pointers in allocas for use later without violating a nocapture
+  // gurantee by the caller, as the allocas are torn down at caller return.
+  // Likewise a leaked malloc would not be accessible outside of the caller,
----------------



================
Comment at: llvm/lib/Transforms/Utils/InferCallsiteAttrs.cpp:357
+  // the caller will apply to the callsites throw. If the caller has a landing
+  // padd, its possible for the callsite to capture a pointer in a throw that
+  // is later cleared by the caller.
----------------



================
Comment at: llvm/lib/Transforms/Utils/InferCallsiteAttrs.cpp:385
+  // attributes from.
+  for (unsigned I = 0; I < Caller->arg_size(); ++I)
+    if (checkCallerHasParamAttr(I, Attribute::NoCapture))
----------------



================
Comment at: llvm/lib/Transforms/Utils/InferCallsiteAttrs.cpp:670
+  const Function *PF = BB->getParent();
+  assert(PF && "Function should never but null in this context");
+
----------------



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152226