Page MenuHomePhabricator

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

Authored by philip.pfaffe on Dec 28 2018, 5:33 AM.

Details

Summary

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

Diff Detail

Repository
rL LLVM

Event Timeline

philip.pfaffe created this revision.Dec 28 2018, 5:33 AM

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?

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

chandlerc added inline comments.Dec 28 2018, 4:27 PM
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.

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.

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?

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

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

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 marked an inline comment as done.

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

chandlerc accepted this revision.Jan 2 2019, 3:33 AM

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 ([&]).

This revision is now accepted and ready to land.Jan 2 2019, 3:33 AM
This revision was automatically updated to reflect the committed changes.