This is an archive of the discontinued LLVM Phabricator instance.

[FuncSpec] Don't specialise call sites that have the MinSize attribute set
ClosedPublic

Authored by SjoerdMeijer on Sep 8 2021, 8:23 AM.

Details

Summary

Not only the function declaration can have the minsize attribute set, but also the call site. I don't think we would like to specialise when a call instruction has minsize set.

Diff Detail

Event Timeline

SjoerdMeijer created this revision.Sep 8 2021, 8:23 AM
SjoerdMeijer requested review of this revision.Sep 8 2021, 8:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 8 2021, 8:23 AM
snehasish added inline comments.Sep 8 2021, 12:07 PM
llvm/lib/Transforms/IPO/FunctionSpecialization.cpp
659

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?

669

nit: Prefer submitting this unrelated style change as an NFC.

ChuanqiXu added inline comments.Sep 8 2021, 7:03 PM
llvm/lib/Transforms/IPO/FunctionSpecialization.cpp
659

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?

SjoerdMeijer added inline comments.Sep 9 2021, 12:58 AM
llvm/lib/Transforms/IPO/FunctionSpecialization.cpp
659

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?

669

Sure, will remove it from here.

ChuanqiXu added inline comments.Sep 9 2021, 1:52 AM
llvm/lib/Transforms/IPO/FunctionSpecialization.cpp
659

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.

690

If we decide not to add a check in the end, we need to add comment to clarify it.

I have left a comment in rewriteCallSites, and added an extra test function-specialization-minsize3.ll that contains 2 calls with only one of them annotated with MinSize to make it even clearer that we only rewrite the callsites that don't have minsize set.

ChuanqiXu accepted this revision.Sep 9 2021, 4:24 AM

LGTM with the comments addressed.

llvm/lib/Transforms/IPO/FunctionSpecialization.cpp
660

We need to mark AllConstant as false before we continue. Otherwise it is possible that the original function may be considered as unreachable incorrectly.

This revision is now accepted and ready to land.Sep 9 2021, 4:24 AM
uabelho added a subscriber: uabelho.Sep 9 2021, 5:44 AM
snehasish accepted this revision.Sep 9 2021, 9:22 AM

LGTM, thanks for adding the additional test case.

SjoerdMeijer added inline comments.Sep 10 2021, 12:37 AM
llvm/lib/Transforms/IPO/FunctionSpecialization.cpp
660

Will add that before committing.

Thanks both for reviewing.

This revision was landed with ongoing or failed builds.Sep 10 2021, 1:04 AM
This revision was automatically updated to reflect the committed changes.