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

[Chuanqi Xu via Phabricator via llvm-commits]

ChuanqiXu added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:659
       auto &CS = *cast<CallBase>(U);
+      if (CS.hasFnAttr(Attribute::MinSize))
+        continue;
----------------
SjoerdMeijer wrote:
> 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?
IMO, the behavior in this patch looks OK to me. I think there 2 cases:
- The callee is marked with `MinSize`. In this case, we should give up specializing it. We've handled this in previous patch.
- The callsite is marked with `MinSize` but the callee isn't. In this case, I think we should ignore the callsite marked `MinSize` when we deciding whether or not should we specialize it. But once we decided to specialize it. I think we could replace the callsites marked with `MinSize`. Since the replacement wouldn't enlarge the size.

In other words, the attribute in callstie only marks the attribute in the callsite only. It wouldn't and shouldn't affect if other transformation outside of the callsite.


================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:690
   /// \p Clone instead.
   void rewriteCallSites(Function *F, Function *Clone, Argument &Arg,
                         Constant *C) {
----------------
If we decide not to add a check in the end, we need to add comment to clarify it.


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

https://reviews.llvm.org/D109441