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 ↗

(On Diff #179629)

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 ↗

(On Diff #179830)

"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

212 ↗

(On Diff #179830)

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.

227 ↗

(On Diff #179830)

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

llvm/trunk/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
-  ///      the existing global.
+  /// If it does not exist, invoke a callback to create a declaration of the
+  /// global and return it. The global is constantexpr casted to the expected
+  /// type if necessary.
+  Constant *
+  getOrInsertGlobal(StringRef Name, Type *Ty,
+                    function_ref<GlobalVariable *()> CreateGlobalCallback);
+
+  /// Look up the specified global in the module symbol table. If required, this
+  /// overload constructs the global variable using its constructor's defaults.
   Constant *getOrInsertGlobal(StringRef Name, Type *Ty);
 
 /// @}
 /// @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

llvm/trunk/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,
+    function_ref<GlobalVariable *()> CreateGlobalCallback) {
   // 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.
-  }
+  if (!GV)
+    GV = CreateGlobalCallback();
+  assert(GV && "The CreateGlobalCallback is expected to create a global");
 
   // 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;
 }
 
+// Overload to construct a global variable using its constructor's defaults.
+Constant *Module::getOrInsertGlobal(StringRef Name, Type *Ty) {
+  return getOrInsertGlobal(Name, Ty, [&] {
+    return new GlobalVariable(*this, Ty, false, GlobalVariable::ExternalLinkage,
+                              nullptr, Name);
+  });
+}
+
 //===----------------------------------------------------------------------===//
 // 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 {