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

[Momchil Velikov via Phabricator via llvm-commits]

chill marked 2 inline comments as done.
chill added inline comments.


================
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));
----------------
SjoerdMeijer wrote:
> Nit: does `C` not always have a value?
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.


================
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
 
----------------
SjoerdMeijer wrote:
> Nit: do we need this change? It might no longer test "compiler crash promote alloca"?
It still tests it, I reverted the fix and verified the compiler still crashes.


================
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.
+
----------------
SjoerdMeijer wrote:
> Nit: this sentence does not really flow, perhaps some punctuation missing.
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.


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

https://reviews.llvm.org/D149476