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

[Noah Goldstein via Phabricator via cfe-commits]

goldstein.w.n marked 8 inline comments as done.
goldstein.w.n added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Utils/InferCallsiteAttrs.h:28
+  // relatively high value is okay.
+  static constexpr unsigned kMaxChecks = UINT_MAX;
+
----------------
arsenm wrote:
> Does changing this to something meaningful change the compile time results?
lowered to 100, doesn't seem to help:
https://llvm-compile-time-tracker.com/compare.php?from=79feb6e78b818e33ec69abdc58c5f713d691554f&to=79e0b7e6f0547de9ad534c02869437160028af01&stat=instructions:u

Lowered to 10, doesn't seem to help either:
https://llvm-compile-time-tracker.com/compare.php?from=79feb6e78b818e33ec69abdc58c5f713d691554f&to=9e3f35bc8b6f0f261698ba1ce18229d1b8d9cdb6&stat=instructions%3Au

I'll drop unless you think there is a reason to keep it around.


================
Comment at: llvm/include/llvm/Transforms/Utils/InferCallsiteAttrs.h:63
+  bool checkCallerHasFnAttr(Attribute::AttrKind Attr) const {
+    return (CxtCB && CxtCB->hasFnAttr(Attr)) || Caller->hasFnAttribute(Attr);
+  };
----------------
arsenm wrote:
> 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
Not quite, so `CxtB` is used to specify propagation of function attributes from a specific callsite. This may be useful in inline. i.e if we have:

foo -> bar -> baz
and
fiz -> bar -> buz

If `bar` in `foo` is getting inlined, we can propagate not only the function attributes of `bar`, but also the callsite attributes of that specific call to `bar` to any callsites (so `baz`).

At the moment its always null in fact.


================
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
----------------
arsenm wrote:
> 
I think callsite is regularly refererred to as singular word (in other files too) so prefer to keep as is. That okay?


================
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.
----------------
arsenm wrote:
> 
did 'pad' -> 'padd' but prefer to keep callsite as one word


================
Comment at: llvm/lib/Transforms/Utils/InferCallsiteAttrs.cpp:692
+  else
+    memset(&CurFnInfo, kMaybe, sizeof(CurFnInfo));
+  Caller = ParentFunc;
----------------
arsenm wrote:
> Not very C++y, use the default constructor?
If `PreserveCache` is true, then we can redo same function (non-sequentially) and get saved analysis. If you think this is meaningless and we will always do one-off functions I can change to just construct with a caller (although I think for inlining this might come up).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152226