Extend Module::getOrInsertGlobal to also take the default arguments accepted by the GlobalVariable constructor.

[philip.pfaffe]

Summary

The default arguments are only used when creating a new global, not when
looking up an existing one.

[compnerd]

I'm not sure how I feel about the additional parameters. What do you think about packaging up the filters into a structure and passing a single "blob" as the filter?

[philip.pfaffe]

The extra arguments are not filters. The idea here is to basicly be a mostly-drop-in replacement for calling the GlobalVariable constructor.

[chandlerc]

llvm/include/llvm/IR/Module.h

411–417

I was going to suggest a different API that I think also addresses compnerd's comment:

Constant *getOrInsertGlobal(StringRef Name, Type *Ty,
                            function_ref<GlobalVariable *()> NewGlobalCallback);

This avoids duplicating all the ways of constructing the global variable and also makes it obvious that they don't participate in the get side of the API.

[philip.pfaffe]

I get the idea of trying to separate query and creation arguments to make their meaning explicit. But how do you expect this to be used? The default call (i.e., the one everyone is using right now), would then look like

M.getOrInsertGlobal("Global", Ctx.getIntTy(), [] { return new GlobalVariable("Global", Ctx.getIntTy()); });

That's super verbose, and to me at least it's much harder to read then just calling Module::getGlobalVariable in an if.

[chandlerc]

In D56130#1342432, @philip.pfaffe wrote:

I get the idea of trying to separate query and creation arguments to make their meaning explicit. But how do you expect this to be used? The default call (i.e., the one everyone is using right now), would then look like

M.getOrInsertGlobal("Global", Ctx.getIntTy(), [] { return new GlobalVariable("Global", Ctx.getIntTy()); });

That's super verbose, and to me at least it's much harder to read then just calling Module::getGlobalVariable in an if.

So add an overload that supplies that call back?

[philip.pfaffe]

That solves the default case, but I feel like the new three-argument case is still not much better than getGlobalVariable and an if.

[chandlerc]

In D56130#1342467, @philip.pfaffe wrote:

That solves the default case, but I feel like the new three-argument case is still not much better than getGlobalVariable and an if.

Yeah, it just makes it clear where any future changes like a mutex

[chandlerc]

In D56130#1342472, @chandlerc wrote:

In D56130#1342467, @philip.pfaffe wrote:

That solves the default case, but I feel like the new three-argument case is still not much better than getGlobalVariable and an if.

Yeah, it just makes it clear where any future changes like a mutex

... Would go.

And it factors the if a bit into common code.

(Also, phone typing is ... Tricky.)

[philip.pfaffe]

Follow @chandlerc's proposal for a much more narrow API.

[chandlerc]

Thanks!

LGTM with small tweaks shown inline.

llvm/include/llvm/IR/Module.h

406–407

"callback to a declaration" -> "callback to create a declaration"?

I think if you follow my suggestion below, you can also simplify this some as in all cases some of the basic stuff will fall out.

llvm/lib/IR/Module.cpp

221

I think we should set GV here but fall through.

I'm imagining the same code that caused us to have this in the first place (creating a global with one type but looking it up with a different but perhaps compatible type) could also easily hit here.

And it should be a no-op for the current users because the types will somewhat obviously Just Match.

235

As this callback should only be run prior to the method returning, I'd either capture nothing ([]) or by reference ([&]).

llvm/include/llvm/IR/Module.h

const GlobalVariable *getNamedGlobal(StringRef Name) const {
     return getGlobalVariable(Name, true);
   }
   GlobalVariable *getNamedGlobal(StringRef Name) {
     return const_cast<GlobalVariable *>(
                        static_cast<const Module *>(this)->getNamedGlobal(Name));
   }
 
   /// Look up the specified global in the module symbol table.
   ///   1. If it does not exist, add a declaration of the global and return it.
   ///   2. Else, the global exists but has the wrong type: return the function
   ///      with a constantexpr cast to the right type.
-  ///   3. Finally, if the existing global is the correct declaration, return
+  ///   3. Finally, if the existing global has the correct type, return
   ///      the existing global.
-  Constant *getOrInsertGlobal(StringRef Name, Type *Ty);
+  /// Note that if optional arguments aren't considered for matching the global.
+  Constant *getOrInsertGlobal(
+      StringRef Name, Type *Ty, bool isConstant = false,
+      GlobalValue::LinkageTypes Linkage = GlobalVariable::ExternalLinkage,
+      Constant *Initializer = nullptr,
+      GlobalValue::ThreadLocalMode TLM = GlobalValue::NotThreadLocal,
+      unsigned AddressSpace = 0, bool isExternallyInitialized = false);
 
   /// @}
   /// @name Global Alias Accessors
   /// @{
 
   /// Return the global alias in the module with the specified name, of
   /// arbitrary type. This method returns null if a global with the specified
   /// name is not found.
   GlobalAlias *getNamedAlias(StringRef Name) const;
 
 /// @}
 /// @name Global IFunc Accessors

llvm/lib/IR/Module.cpp

 }
 
 /// getOrInsertGlobal - Look up the specified global in the module symbol table.
 ///   1. If it does not exist, add a declaration of the global and return it.
 ///   2. Else, the global exists but has the wrong type: return the function
 ///      with a constantexpr cast to the right type.
 ///   3. Finally, if the existing global is the correct declaration, return the
 ///      existing global.
-Constant *Module::getOrInsertGlobal(StringRef Name, Type *Ty) {
+Constant *Module::getOrInsertGlobal(StringRef Name, Type *Ty, bool isConstant,
+                                    GlobalValue::LinkageTypes Linkage,
+                                    Constant *Initializer,
+                                    GlobalValue::ThreadLocalMode TLM,
+                                    unsigned AddressSpace,
+                                    bool isExternallyInitialized) {
   // See if we have a definition for the specified global already.
   GlobalVariable *GV = dyn_cast_or_null<GlobalVariable>(getNamedValue(Name));
   if (!GV) {
     // Nope, add it
     GlobalVariable *New =
-      new GlobalVariable(*this, Ty, false, GlobalVariable::ExternalLinkage,
-                         nullptr, Name);
-     return New;                    // Return the new declaration.
+        new GlobalVariable(*this, Ty, isConstant, Linkage, Initializer, Name,
+                           nullptr, TLM, AddressSpace, isExternallyInitialized);
+    return New; // Return the new declaration.
   }
 
   // If the variable exists but has the wrong type, return a bitcast to the
   // right type.
   Type *GVTy = GV->getType();
   PointerType *PTy = PointerType::get(Ty, GVTy->getPointerAddressSpace());
   if (GVTy != PTy)
     return ConstantExpr::getBitCast(GV, PTy);
 
   // Otherwise, we just found the existing function or a prototype.
   return GV;
 }
 
 //===----------------------------------------------------------------------===//
 // Methods for easy access to the global variables in the module.
 //
 
 // getNamedAlias - Look up the specified global in the module symbol table.
 // If it does not exist, return null.
 //
 GlobalAlias *Module::getNamedAlias(StringRef Name) const {
   return dyn_cast_or_null<GlobalAlias>(getNamedValue(Name));
 }