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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
