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

[SjoerdMeijer]

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.

[snehasish]

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]

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]

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]

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.

[SjoerdMeijer]

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]

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.

[snehasish]

LGTM, thanks for adding the additional test case.

[SjoerdMeijer]

llvm/lib/Transforms/IPO/FunctionSpecialization.cpp

660

Will add that before committing.

Thanks both for reviewing.

llvm/lib/Transforms/IPO/FunctionSpecialization.cpp

bool getPossibleConstants(Argument *A,
     Function *F = A->getParent();
     bool AllConstant = true;
 
     // Iterate over all the call sites of the argument's parent function.
     for (User *U : F->users()) {
       if (!isa<CallInst>(U) && !isa<InvokeInst>(U))
         continue;
       auto &CS = *cast<CallBase>(U);
+      if (CS.hasFnAttr(Attribute::MinSize))
+        continue;
 
       // If the parent of the call site will never be executed, we don't need
       // to worry about the passed value.
       if (!Solver.isBlockExecutable(CS.getParent()))
         continue;
 
       auto *V = CS.getArgOperand(A->getArgNo());
       // 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())
           return false;
-        }
-      }
 
       if (isa<Constant>(V) && (Solver.getLatticeValueFor(V).isConstant() ||
                                EnableSpecializationForLiteralConstant))
         Constants.push_back(cast<Constant>(V));
       else
         AllConstant = false;
     }
 
     // If the argument can only take on constant values, AllConstant will be
     // true.
     return AllConstant;
   }
 
   /// Rewrite calls to function \p F to call function \p Clone instead.
   ///
   /// This function modifies calls to function \p F whose argument at index \p
   /// ArgNo is equal to constant \p C. The calls are rewritten to call function
   /// \p Clone instead.
   void rewriteCallSites(Function *F, Function *Clone, Argument &Arg,
                         Constant *C) {
     unsigned ArgNo = Arg.getArgNo();
     SmallVector<CallBase *, 4> CallSitesToRewrite;
     for (auto *U : F->users()) {
       if (!isa<CallInst>(U) && !isa<InvokeInst>(U))
         continue;
       auto &CS = *cast<CallBase>(U);
       if (!CS.getCalledFunction() || CS.getCalledFunction() != F)

llvm/test/Transforms/FunctionSpecialization/function-specialization-minsize2.ll

+; RUN: opt -function-specialization -func-specialization-size-threshold=3 -S < %s | FileCheck %s
+
+; CHECK-NOT: @compute.1
+; CHECK-NOT: @compute.2
+
+define i64 @main(i64 %x, i1 %flag) {
+entry:
+  br i1 %flag, label %plus, label %minus
+
+plus:
+  %tmp0 = call i64 @compute(i64 %x, i64 (i64)* @plus) #0
+  br label %merge
+
+minus:
+  %tmp1 = call i64 @compute(i64 %x, i64 (i64)* @minus) #0
+  br label %merge
+
+merge:
+  %tmp2 = phi i64 [ %tmp0, %plus ], [ %tmp1, %minus]
+  ret i64 %tmp2
+}
+
+define internal i64 @compute(i64 %x, i64 (i64)* %binop) {
+entry:
+  %tmp0 = call i64 %binop(i64 %x)
+  ret i64 %tmp0
+}
+
+define internal i64 @plus(i64 %x) {
+entry:
+  %tmp0 = add i64 %x, 1
+  ret i64 %tmp0
+}
+
+define internal i64 @minus(i64 %x) {
+entry:
+  %tmp0 = sub i64 %x, 1
+  ret i64 %tmp0
+}
+
+attributes #0 = { minsize optsize }