[FuncSpec] Specialising on addresses of const global values

[SjoerdMeijer]

Summary

This introduces an option to allow specialising on the address of global values. This option is off by default because it is likely not that profitable to do so and needs more investigation. Before, we were specialising on addresses and thus this changes the default behaviour.

[nikic]

Shouldn't this be specializing on the address of the global rather than its initializer?

[SjoerdMeijer]

Thanks for commenting!
I think we want to specialise on the constant value that is passed to the value. If we specialise on the address, then I think we could get wrong results as that global may be modified. Tomorrow morning I will write some more tests to convince myself this is true. In the mean time I have regenerated the test that shows the before/after a little bit better.

[ChuanqiXu]

This looks good to me. Although we could specialize on the address, the benefit seems little. We couldn't remove the loads. We could only eliminate one argument. Although it is possible that the user computes the address as integer, in which case specialize on address could be beneficial, it seems really rare.

[SjoerdMeijer]

I have precommitted the test in rGa4e437e3c959.

Specialising on the address confused me, i.e. I had not yet consciously thought about allowing/disallowing it. But now having looked a bit more into this, I don't see the problem, and I don't see why we would disallow it with this patch. So think I will abandon this patch. Any thoughts?

[ChuanqiXu]

In D109775#3003332, @SjoerdMeijer wrote:

I have precommitted the test in rGa4e437e3c959.

Specialising on the address confused me, i.e. I had not yet consciously thought about allowing/disallowing it. But now having looked a bit more into this, I don't see the problem, and I don't see why we would disallow it with this patch. So think I will abandon this patch. Any thoughts?

I prefer to disallowing specializing on the address of variable globals. Since it looks like not beneficial in most cases. And if we could be sure that it wouldn't be beneficial, I think we could disallow such cases.

[nikic]

In D109775#3003343, @ChuanqiXu wrote:

In D109775#3003332, @SjoerdMeijer wrote:

I have precommitted the test in rGa4e437e3c959.

Specialising on the address confused me, i.e. I had not yet consciously thought about allowing/disallowing it. But now having looked a bit more into this, I don't see the problem, and I don't see why we would disallow it with this patch. So think I will abandon this patch. Any thoughts?

I prefer to disallowing specializing on the address of variable globals. Since it looks like not beneficial in most cases. And if we could be sure that it wouldn't be beneficial, I think we could disallow such cases.

Shouldn't that be a cost model decision though? I agree that it likely won't be beneficial, but it it's not like it can't be (say you have an icmp eq %arg, @g in the specialized function).

[ChuanqiXu]

In D109775#3003347, @nikic wrote:

In D109775#3003343, @ChuanqiXu wrote:

In D109775#3003332, @SjoerdMeijer wrote:

I have precommitted the test in rGa4e437e3c959.

Specialising on the address confused me, i.e. I had not yet consciously thought about allowing/disallowing it. But now having looked a bit more into this, I don't see the problem, and I don't see why we would disallow it with this patch. So think I will abandon this patch. Any thoughts?

I prefer to disallowing specializing on the address of variable globals. Since it looks like not beneficial in most cases. And if we could be sure that it wouldn't be beneficial, I think we could disallow such cases.

Shouldn't that be a cost model decision though? I agree that it likely won't be beneficial, but it it's not like it can't be (say you have an icmp eq %arg, @g in the specialized function).

Oh yeah, right, it would be absolutely better to filter the not beneficial cases in the cost model. I agree that it is a better direction.

[SjoerdMeijer]

Ok, good points. I will introduce an option to enable/disable specialising on addresses that is off by default, to make explicit and 'document' that this is possible (and give the opportunity to turn this on).

[SjoerdMeijer]

Added an option that is off by default to specialise on addresses. Investigating a cost model for this is for another day, I don't want to do this at this point.

[ChuanqiXu]

LGTM

[SjoerdMeijer]

Thanks for reviewing!

llvm/lib/Transforms/IPO/FunctionSpecialization.cpp

cl::desc("Don't specialize functions that have less than this theshold "
              "number of instructions"),
     cl::init(100));
 
 static cl::opt<unsigned>
     AvgLoopIterationCount("func-specialization-avg-iters-cost", cl::Hidden,
                           cl::desc("Average loop iteration count cost"),
                           cl::init(10));
 
+static cl::opt<bool> SpecializeOnAddresses(
+    "func-specialization-on-address", cl::init(false), cl::Hidden,
+    cl::desc("Enable function specialization on the address of global values"));
+
 // TODO: This needs checking to see the impact on compile-times, which is why
 // this is off by default for now.
 static cl::opt<bool> EnableSpecializationForLiteralConstant(
     "function-specialization-for-literal-constant", cl::init(false), cl::Hidden,
     cl::desc("Enable specialization of functions that take a literal constant "
              "as an argument."));
 
 // Helper to check if \p LV is either a constant or a constant
for (User *U : F->users()) {
 
       // 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 (auto *GV = dyn_cast<GlobalVariable>(V)) {
+        // Check if we want to specialize on the address of non-constant
+        // global values.
+        if (!GV->isConstant())
+          if (!SpecializeOnAddresses)
+            return false;
+
         if (!GV->getValueType()->isSingleValueType())
           return false;
+      }
 
       if (isa<Constant>(V) && (Solver.getLatticeValueFor(V).isConstant() ||
                                EnableSpecializationForLiteralConstant))
         Constants.push_back(cast<Constant>(V));
       else
         AllConstant = false;
     }
 

llvm/test/Transforms/FunctionSpecialization/function-specialization-nonconst-glob.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+
 ; RUN: opt -function-specialization -force-function-specialization -S < %s | FileCheck %s
+; RUN: opt -function-specialization -force-function-specialization -func-specialization-on-address=0 -S < %s | FileCheck %s
+; RUN: opt -function-specialization -force-function-specialization -func-specialization-on-address=1 -S < %s | FileCheck %s --check-prefix=ON-ADDRESS
+
+; Global B is not constant. We do not specialise on addresses unless we
+; enable that:
 
-; Global B is not constant. We do specialise on this global, showing we
-; specialise on its address.
+; ON-ADDRESS: call i32 @foo.1(i32 %x, i32* @A)
+; ON-ADDRESS: call i32 @foo.2(i32 %y, i32* @B)
 
 target datalayout = "e-m:e-i8:8:32-i16:16:32-i64:64-i128:128-n32:64-S128"
 
 @A = external dso_local constant i32, align 4
 @B = external dso_local global i32, align 4
 
 define dso_local i32 @bar(i32 %x, i32 %y) {
 ; CHECK-LABEL: @bar(
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    [[TOBOOL:%.*]] = icmp ne i32 [[X:%.*]], 0
 ; CHECK-NEXT:    br i1 [[TOBOOL]], label [[IF_THEN:%.*]], label [[IF_ELSE:%.*]]
 ; CHECK:       if.then:
 ; CHECK-NEXT:    [[CALL:%.*]] = call i32 @foo.1(i32 [[X]], i32* @A)
 ; CHECK-NEXT:    br label [[RETURN:%.*]]
 ; CHECK:       if.else:
-; CHECK-NEXT:    [[CALL1:%.*]] = call i32 @foo.2(i32 [[Y:%.*]], i32* @B)
+; CHECK-NEXT:    [[CALL1:%.*]] = call i32 @foo(i32 [[Y:%.*]], i32* @B)
 ; CHECK-NEXT:    br label [[RETURN]]
 ; CHECK:       return:
 ; CHECK-NEXT:    [[RETVAL_0:%.*]] = phi i32 [ [[CALL]], [[IF_THEN]] ], [ [[CALL1]], [[IF_ELSE]] ]
 ; CHECK-NEXT:    ret i32 [[RETVAL_0]]
 ;
 entry:
   %tobool = icmp ne i32 %x, 0
   br i1 %tobool, label %if.then, label %if.else
 
 if.then:
   %call = call i32 @foo(i32 %x, i32* @A)
   br label %return
 
 if.else:
   %call1 = call i32 @foo(i32 %y, i32* @B)
   br label %return
 
 return:
   %retval.0 = phi i32 [ %call, %if.then ], [ %call1, %if.else ]
   ret i32 %retval.0
 }
 
 define internal i32 @foo(i32 %x, i32* %b) {
-; CHECK-LABEL: @foo(
+; CHECK-LABEL: define internal i32 @foo(
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    [[TMP0:%.*]] = load i32, i32* [[B:%.*]], align 4
 ; CHECK-NEXT:    [[ADD:%.*]] = add nsw i32 [[X:%.*]], [[TMP0]]
 ; CHECK-NEXT:    ret i32 [[ADD]]
 ;
 entry:
   %0 = load i32, i32* %b, align 4
   %add = add nsw i32 %x, %0
   ret i32 %add
 }
 
 ; CHECK-LABEL: define internal i32 @foo.1(i32 %x, i32* %b) {
 ; CHECK-NEXT:  entry:
 ; CHECK-NEXT:    %0 = load i32, i32* @A, align 4
 ; CHECK-NEXT:    %add = add nsw i32 %x, %0
 ; CHECK-NEXT:    ret i32 %add
 ; CHECK-NEXT:  }
 
-; CHECK-LABEL: define internal i32 @foo.2(i32 %x, i32* %b) {
-; CHECK-NEXT:  entry:
-; CHECK-NEXT:    %0 = load i32, i32* @B, align 4
-; CHECK-NEXT:    %add = add nsw i32 %x, %0
-; CHECK-NEXT:    ret i32 %add
-; CHECK-NEXT:  }
+; CHECK-NOT: define internal i32 @foo.2(