[PATCH] D109441: [FuncSpec] Don't specialise call sites that have the MinSize attribute set

Sjoerd Meijer via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Sep 9 00:58:25 PDT 2021


SjoerdMeijer added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:659
       auto &CS = *cast<CallBase>(U);
+      if (CS.hasFnAttr(Attribute::MinSize))
+        continue;
----------------
ChuanqiXu wrote:
> snehasish wrote:
> > If I understand correctly this should only disallow specialization of this callsite as opposed to the function if any callsite is marked minsize. This is aligned with the intent of minsize applied at the callsite. Do we need to add a similar check to rewriteCallSites?
> It is an interesting question that if the attribute marked on callsite should be applied to callee function. Since it is possible that there is an attribute in callsite only. So here is the decision point:
> - if the attribute marked on callsite should be applied to callee function. We should decide to not specialize the function once we found that there is callsite of it marked as `MinSize`.
> - if the attribute marked on callsite could not be applied to callee function. I think current implementation is right. A question is that should we add a similar check in `rewriteCallSites `. I think we can omit it. Since once the function get specialized, replace the corresponding callsite wouldn't enlarge the size any more.
> 
> ---
> 
> Another funny question is that if the attribute marked on callsite should be applied to callee function. How should we consider indirect call. I mean, once we found an indirect call with specific attribute, should we do alias analysis to check how many functions should this indirect call could be?
Yeah, I also found this an interesting case and I wasn't entirely sure about what the behaviour should be. In general, I think we have 3 combinations:
- minsize is set for both callsite and callee,
- it's only set for the callsite,
- it's only set for the callee.

But looking and thinking about this case, when minsize is only set for the callsite, we would still specialise functions. I think I would find that result surprising as we would increase code-size despite the minsize attribute on the call instruction.

So, with this patch, we don't specialise when minsize is when one of the callsite or callee has minsize, or both (and now we capture all 3 cases).

What do you think about this?


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:669
       // TrackValueOfGlobalVariable only tracks scalar global variables.
-      if (auto *GV = dyn_cast<GlobalVariable>(V)) {
-        if (!GV->getValueType()->isSingleValueType()) {
+      if (auto *GV = dyn_cast<GlobalVariable>(V))
+        if (!GV->getValueType()->isSingleValueType())
----------------
snehasish wrote:
> nit: Prefer submitting this unrelated style change as an NFC.
Sure, will remove it from here.


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

https://reviews.llvm.org/D109441



More information about the llvm-commits mailing list