This is an archive of the discontinued LLVM Phabricator instance.

Move ownership of GCStrategy objects to LLVMContext
ClosedPublic

Authored by reames on Dec 30 2014, 9:06 AM.

Details

Summary

(I would appreciate a close review on this one. I'm changing ownership in a non-trivial way and am introducing what might be considered a layering violation - IR owrning and returning pointers to a CodeGen class.)

Rather than have the GCStrategy object owned by the GCModuleInfo - which is an immutable analysis pass used mainly by gc.root - have it be owned by the LLVMContext. This simplifies the ownership logic (i.e. can you have two instances of the same strategy at once?), but more importantly, allows us to access the GCStrategy in the middle end optimizer. To this end, I add an accessor through Function which becomes the canonical way to get at a GCStrategy instance.

In the near future, this will allows me to move some of the checks from http://reviews.llvm.org/D6808 into the Verifier itself, and to introduce optimization legality predicates for some of the recent additions to InstCombine. (These will follow as separate changes.)

Diff Detail

Event Timeline

reames updated this revision to Diff 17721.Dec 30 2014, 9:06 AM
reames retitled this revision from to Move ownership of GCStrategy objects to LLVMContext.
reames updated this object.
reames edited the test plan for this revision. (Show Details)
reames added reviewers: chandlerc, nicholas, sanjoy.
reames added a subscriber: Unknown Object (MLST).

A small review, fwiw.

include/llvm/CodeGen/GCMetadata.h
167

Why didn't you preserve std::unique_ptr?

170

You have a GCStrategyList, GCStrategyMap, and StrategyMap? Makes me wonder what happened to the corresponding StrategyList.

include/llvm/CodeGen/GCStrategy.h
54

This is unrelated to the overall change, isn't it? (Just asking)

70

So this is the big change. GCModuleInfo benefited from this friendship by being able to instantiate and Name strategies from the GCRegistry. Now, you've removed that code and put it in ContextImpl.

lib/CodeGen/GCMetadata.cpp
80

There is no user of this StrategyList. The only iterator I can see is in getGCStrategy(), and that uses GCStrategyList. What is this for?

lib/IR/LLVMContextImpl.cpp
16

Some final comments:
If I understand correctly, your patch helps us verify and optimize the code generated by gc plugins, hence making their lives easier.

reames added inline comments.Jan 5 2015, 10:40 AM
include/llvm/CodeGen/GCMetadata.h
167

The GCStrategy is no longer owned by the GCModuleInfo.

170

Sorry, what? I don't understand your comment.

include/llvm/CodeGen/GCStrategy.h
54

Not really. I'm removing headers which were only required by the deleted code and adding back a header which was an indirect dependency.

lib/CodeGen/GCMetadata.cpp
80

Again, what? There is no GCStrategyList member variable.

lib/IR/LLVMContextImpl.cpp
16

I'd frame this differently: "it helps us verify and optimize code compiled for targets with a garbage collector". But the difference is minor.

sanjoy edited edge metadata.Jan 5 2015, 11:02 AM

Some minor nits inline. I think the ownership change is okay, but I'm not confident enough in that judgement to LGTM it.

lib/IR/Function.cpp
393

LLVM style says LLVMContextImpl *pImpl and const char *Name.

393

Why not just getContext().pImpl?

lib/IR/LLVMContextImpl.cpp
177

ProgrammersManual.rst says "`StringRef` is small and pervasive enough in LLVM that it should always be passed by value."

188

Not part of the review, but is this complexity (the StringMap) actually needed? I'd think the usual case has just one GC strategy.

artagnon added inline comments.Jan 5 2015, 12:00 PM
lib/CodeGen/GCMetadata.cpp
80

I've probably been staring at this for too long, but StrategyList doesn't really seem to be looked up or iterated over anywhere. Can you show me a counterexample? Dump from a git-grep:

artagnon|gc-relocate>:~/src/llvm$ g g StrategyList
include/llvm/CodeGen/GCMetadata.h=43=namespace llvm {
include/llvm/CodeGen/GCMetadata.h:171:    list_type StrategyList;
include/llvm/CodeGen/GCMetadata.h:207:    iterator begin() const { return StrategyList.begin(); }
include/llvm/CodeGen/GCMetadata.h:208:    iterator end()   const { return StrategyList.end();   }
lib/CodeGen/GCMetadata.cpp=64=GCStrategy *GCModuleInfo::getOrCreateStrategy(const Module *M,
lib/CodeGen/GCMetadata.cpp:76:      StrategyList.push_back(std::move(S));
lib/CodeGen/GCMetadata.cpp:77:      return StrategyList.back().get();
lib/CodeGen/GCMetadata.cpp=100=void GCModuleInfo::clear() {
lib/CodeGen/GCMetadata.cpp:104:  StrategyList.clear();
artagnon added inline comments.Jan 5 2015, 12:07 PM
lib/CodeGen/GCMetadata.cpp
80

Uh, never mind. GCModuleInfo::iterator is used in LowerIntrinsics::doInitialization() and AsmPrinter::doFinalization().

Sorry about that.

*goes to get more coffee*

reames updated this revision to Diff 17869.Jan 7 2015, 12:03 PM
reames edited edge metadata.

Address Sanjoy's style comments and add some header includes to fix a build issue on TOT.

ping - specifically regards to the ownership, everything else has been reviewed

ping

This is blocking work I'd like to get in before we cut 3.6. The only
part left to review is having someone look over the ownership changes
for GCStrategy and make sure I haven't missed something. The code has
already been reviewed, just not by someone who felt comfortable with the
ownership changes.

If I haven't received a response on the ownership changes by tomorrow,
I'm going to go ahead and submit. I know this is not generally accepted
practice, but given the code has been reviewed, if not with the emphasis
I'd prefer, and that the change itself is fairly simple, I'm going to
submit unless someone objects.

Philip

nicholas edited edge metadata.Jan 12 2015, 11:18 PM

Philip Reames wrote:

Hi chandlerc, nicholas, sanjoy,

(I would appreciate a close review on this one. I'm changing ownership in a non-trivial way and am introducing what might be considered a layering violation - IR owrning and returning pointers to a CodeGen class.)

Sorry but that's a non-starter right there. We can't have IR depending
on Codegen (unless you want to make Codegen not depend on IR).

We can cheat a little; we could move GCStrategy.h into the IR and
forward-declare MachineFunction and pass it by pointer to
findCustomSafePoints.

Should there be a split, GCStrategy (IR) and MachineGCStrategy? There
isn't enough IR-side user of the new API for me to judge how to deal
with it, even D6808 pretty much just uses isGCManagedPointer.

+ for (GCRegistry::iterator I = GCRegistry::begin(),
+ E = GCRegistry::end(); I != E; ++I) {

Range-based for loop?

Rather than have the GCStrategy object owned by the GCModuleInfo - which is an immutable analysis pass used mainly by gc.root - have it be owned by the LLVMContext. This simplifies the ownership logic (i.e. can you have two instances of the same strategy at once?), but more importantly, allows us to access the GCStrategy in the middle end optimizer. To this end, I add an accessor through Function which becomes the canonical way to get at a GCStrategy instance.

In the near future, this will allows me to move some of the checks from http://reviews.llvm.org/D6808 into the Verifier itself, and to introduce optimization legality predicates for some of the recent additions to InstCombine. (These will follow as separate changes.)

http://reviews.llvm.org/D6811

Files:

include/llvm/CodeGen/GCMetadata.h
include/llvm/CodeGen/GCStrategy.h
include/llvm/IR/Function.h
lib/CodeGen/GCMetadata.cpp
lib/IR/Function.cpp
lib/IR/LLVMContextImpl.cpp
lib/IR/LLVMContextImpl.h

EMAIL PREFERENCES

http://reviews.llvm.org/settings/panel/emailpreferences/
reames updated this revision to Diff 18263.Jan 15 2015, 2:43 PM
reames edited edge metadata.

Updates based on Chandlers comments. GCStrategy is completely moved into IR. I haven't gone through and fixed all the comments yet, but will do so once this lands.

Note that this is dependent on http://reviews.llvm.org/D7004 and that supporting refactoring (splitting GCStrategy.cpp into two files) already landing.

I would also like to get rid of the performCustomLowering mechanism, but that should not block this and will require a bit more thought.

chandlerc edited edge metadata.Jan 15 2015, 3:01 PM

Ok, I think this is looking pretty good in general.

I have one major question left. Does this need to be baked into the IR / context? My *impression* is that the goal here is really just to cache the formation of the GCStrategy object which happens by parsing an attribute. Do I understand this correctly?

If so, have you looked at possibly making this a function analysis that has a cache of these which it populates exactly the same way you populate the context? That should still allow you to use essentially the same lookup path here.

The reason I suggest this approach is that it helps simplify the core IR a bit by letting the IR just deal in an opaque attribute and relying on an analysis to reason about it. I think the code would be almost identical to what you have here just shuffled around a bit. I'd be happy with that either as the first version or with that happening as the immediate follow-up refactoring after this patch.

Also a few trivial comments inline.

include/llvm/CodeGen/GCMetadata.h
46

Stale forward declaration.

157–160

I would nuke the typedef (or at least fix its name) at this point.

Also, "no longer" doesn't really make sense in a comment where there is no visible history.

lib/IR/LLVMContextImpl.cpp
183–184

Range based loop?

So, I don't know the GC code at all and am assuming that GCStrategy.{h,cpp} are just moves of existing code. But provided that there is nothing interesting about the GC side of this and my understanding of the move is correct, this change looks good with the comments on the changed (not moved) code addressed. Two more minor ones below.

lib/CodeGen/GCMetadata.cpp
74–75

Use report_fatal_error here.

lib/IR/Function.cpp
392–393

This variable doesn't seem to be adding much...

This revision was automatically updated to reflect the committed changes.