[PATCH] D149476: [FuncSpec] Fix inconsistent treatment of global variables

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer accepted this revision.
SjoerdMeijer added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: llvm/lib/Transforms/IPO/FunctionSpecialization.cpp:744
+  // global variable, unless explicitly enabled.
+  if (C && C->getType()->isPointerTy() && !C->isNullValue())
+    if (auto *GV = dyn_cast<GlobalVariable>(getUnderlyingObject(C));
----------------
Nit: does `C` not always have a value?


================
Comment at: llvm/test/Transforms/FunctionSpecialization/compiler-crash-promote-alloca.ll:7
 
- at block = internal global [8 x i8] zeroinitializer, align 1
+ at block = internal constant [8 x i8] zeroinitializer, align 1
 
----------------
Nit: do we need this change? It might no longer test "compiler crash promote alloca"?


================
Comment at: llvm/test/Transforms/FunctionSpecialization/global-var-constants.ll:50
+; Check if specialisation on the address of a non-const global variable is not allowed
+; then it is not performed.
+
----------------
Nit: this sentence does not really flow, perhaps some punctuation missing.


================
Comment at: llvm/test/Transforms/FunctionSpecialization/global-var-constants.ll:77
+; GLOBALS-LABEL: define i32 @h2()
+; GLOBALS: call i32 @f.{{[1-9]+}}()
+
----------------
Nit: just check the numeric value here and also above on line 74?


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

https://reviews.llvm.org/D149476