[FuncSpec] Fix inconsistent treatment of global variables

[chill]

Summary

There are a few inaccuracies with how FuncSpec handles global variables.

When specialisation on non-const global variables is disabled (the default)
the pass could nevertheless perform some specializations, e.g. on a constant
GEP expression, or on a SSA variable, for which the Solver has determined
it has the value of a global variable.

When specialisation on non-const global variables is enabled, the pass would
skip non-scalars, e.g. a global array, but this should be completely inconsequential,
a pointer is a pointer.

[SjoerdMeijer]

LGTM

llvm/lib/Transforms/IPO/FunctionSpecialization.cpp

744

Nit: does C not always have a value?

llvm/test/Transforms/FunctionSpecialization/compiler-crash-promote-alloca.ll

7

Nit: do we need this change? It might no longer test "compiler crash promote alloca"?

llvm/test/Transforms/FunctionSpecialization/global-var-constants.ll

51

Nit: this sentence does not really flow, perhaps some punctuation missing.

78

Nit: just check the numeric value here and also above on line 74?

[chill]

llvm/lib/Transforms/IPO/FunctionSpecialization.cpp

744

It's fairly typical for C to be nullptr here, for example we call this functions with a parameter value, which is not a constant by itself, nor it is derived by the solver to be constant.

llvm/test/Transforms/FunctionSpecialization/compiler-crash-promote-alloca.ll

7

It still tests it, I reverted the fix and verified the compiler still crashes.

llvm/test/Transforms/FunctionSpecialization/global-var-constants.ll

51

All right, it might be a clumsy way to say the compiler does what it is requested to do.

What the compiler did with regard to non-const globals was that sometimes
such specialisations were:

• not allowed -> performed
• allowed -> not performed (even though other conditions were present)

The two comments, basically say:

• not allowed -> not performed
• allowed -> performed

It needs a comma before "then", I suppose.

llvm/lib/Transforms/IPO/FunctionSpecialization.cpp

 #include "llvm/Transforms/IPO/FunctionSpecialization.h"
 #include "llvm/ADT/Statistic.h"
 #include "llvm/Analysis/CodeMetrics.h"
 #include "llvm/Analysis/InlineCost.h"
 #include "llvm/Analysis/LoopInfo.h"
 #include "llvm/Analysis/TargetTransformInfo.h"
 #include "llvm/Analysis/ValueLattice.h"
 #include "llvm/Analysis/ValueLatticeUtils.h"
+#include "llvm/Analysis/ValueTracking.h"
 #include "llvm/IR/IntrinsicInst.h"
 #include "llvm/Transforms/Scalar/SCCP.h"
 #include "llvm/Transforms/Utils/Cloning.h"
 #include "llvm/Transforms/Utils/SCCPSolver.h"
 #include "llvm/Transforms/Utils/SizeOpts.h"
 #include <cmath>
 
 using namespace llvm;

 }
 
 /// Check if the value \p V  (an actual argument) is a constant or can only
 /// have a constant value. Return that constant.
 Constant *FunctionSpecializer::getCandidateConstant(Value *V) {
   if (isa<PoisonValue>(V))
     return nullptr;
 
-  // TrackValueOfGlobalVariable only tracks scalar global variables.
-  if (auto *GV = dyn_cast<GlobalVariable>(V)) {
-    // Check if we want to specialize on the address of non-constant
-    // global values.
-    if (!GV->isConstant() && !SpecializeOnAddress)
-      return nullptr;
-
-    if (!GV->getValueType()->isSingleValueType())
-      return nullptr;
-  }
-
   // Select for possible specialisation values that are constants or
   // are deduced to be constants or constant ranges with a single element.
   Constant *C = dyn_cast<Constant>(V);
   if (!C)
     C = Solver.getConstantOrNull(V);
+
+  // Don't specialize on (anything derived from) the address of a non-constant
+  // global variable, unless explicitly enabled.
+  if (C && C->getType()->isPointerTy() && !C->isNullValue())
+    if (auto *GV = dyn_cast<GlobalVariable>(getUnderlyingObject(C));
+        GV && !(GV->isConstant() || SpecializeOnAddress))
+      return nullptr;
+
   return C;
 }
 
 void FunctionSpecializer::updateCallSites(Function *F, const Spec *Begin,
                                           const Spec *End) {
   // Collect the call sites that need updating.
   SmallVector<CallBase *> ToUpdate;
   for (User *U : F->users())

llvm/test/Transforms/FunctionSpecialization/compiler-crash-promote-alloca.ll

 ; RUN: opt -S --passes="ipsccp<func-spec>" -force-specialization < %s | FileCheck %s
 
 ; Tests that `bar` has been specialized and that the compiler did not crash
 ; while attempting to promote the alloca in `entry`.
 ; CHECK: bar.1
 
-@block = internal global [8 x i8] zeroinitializer, align 1
+@block = internal constant [8 x i8] zeroinitializer, align 1
 
 define dso_local void @entry() {
   %1 = alloca i32, align 4
   call void @foo(ptr nonnull %1)
   ret void
 }
 
 define internal void @foo(ptr nocapture readnone %0) {

llvm/test/Transforms/FunctionSpecialization/global-var-constants.ll

+; RUN:  opt -S --passes=ipsccp,deadargelim --force-specialization                       < %s | FileCheck %s --check-prefix=NO-GLOBALS
+; RUN:  opt -S --passes=ipsccp,deadargelim --force-specialization --funcspec-on-address < %s | FileCheck %s --check-prefix=GLOBALS
+@G = global [10 x i32] zeroinitializer, align 4
+
+define internal i32 @f(ptr %p) noinline {
+entry:
+  %0 = load i32, ptr %p, align 4
+  store i32 0, ptr %p, align 4
+  ret i32 %0
+}
+
+define internal i32 @g(i32 %x, i32 %y, ptr %p) noinline {
+entry:
+  %cmp = icmp sgt i32 %x, %y
+  br i1 %cmp, label %if.then, label %if.else
+
+if.then:
+  br label %if.end
+
+if.else:
+  br label %if.end
+
+if.end:
+  %x.addr.0 = phi i32 [ %x, %if.then ], [ 11, %if.else ]
+  %p.addr.0 = phi ptr [ @G, %if.then ], [ %p, %if.else ]
+  %call = call i32 @f(ptr %p.addr.0)
+  %add = add nsw i32 %call, %x.addr.0
+  ret i32 %add
+}
+
+define i32 @h0(ptr %p) {
+entry:
+  %call = call i32 @g(i32 2, i32 1, ptr %p)
+  ret i32 %call
+}
+
+define i32 @h1() {
+entry:
+  %call = call i32 @f(ptr @G)
+  ret i32 %call
+}
+
+define i32 @h2() {
+entry:
+  %call = call i32 @f(ptr getelementptr inbounds (i32, ptr @G, i64 1))
+  ret i32 %call
+}
+
+; Check if specialisation on the address of a non-const global variable
+; is not allowed, then it is not performed.
+
+; NO-GLOBALS-LABEL: define internal i32 @g()
+; NO-GLOBALS: call i32 @f(ptr @G)
+
+; NO-GLOBALS-LABEL: define i32 @h0(ptr %p)
+; NO-GLOBALS:call i32 @g()
+
+; NO-GLOBALS-LABEL: define i32 @h1()
+; NO-GLOBALS: call i32 @f(ptr @G)
+
+; NO-GLOBALS-LABEL: define i32 @h2()
+; NO-GLOBALS: call i32 @f(ptr getelementptr inbounds (i32, ptr @G, i64 1))
+
+; Check if specialisation on the address of a non-const global variable
+; is allowed, then it is performed where possible.
+
+; GLOBALS-LABEL: define internal i32 @g()
+; GLOBALS: call i32 @f.2()
+
+; GLOBALS-LABEL: define i32 @h0(ptr %p)
+; GLOBALS: call i32 @g()
+
+; GLOBALS-LABEL: define i32 @h1()
+; GLOBALS: call i32 @f.2()
+
+; GLOBALS-LABEL: define i32 @h2()
+; GLOBALS: call i32 @f.1()
+