Move ownership of GCStrategy objects to LLVMContext

[reames]

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.)

[artagnon]

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]

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]

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]

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]

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]

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

[reames]

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]

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]

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]

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?

[chandlerc]

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...

include/llvm/CodeGen/GCMetadata.h

   /// Records both the function level information used by GCRoots and a
   /// cache of the 'active' gc strategy objects for the current Module.
   class GCModuleInfo : public ImmutablePass {
-    typedef StringMap<GCStrategy*> strategy_map_type;
-    typedef std::vector<std::unique_ptr<GCStrategy>> list_type;
-
-    strategy_map_type StrategyMap;
+    typedef std::vector<GCStrategy*> list_type;
+    /// A list of GCStrategies which are active in this Module.  These are no
+    /// longer owning pointers.
     list_type StrategyList;
-
-    GCStrategy *getOrCreateStrategy(const Module *M, const std::string &Name);
-
   public:
     /// List of per function info objects.  In theory, Each of these
     /// may be associated with a different GC.

include/llvm/CodeGen/GCStrategy.h

 #define LLVM_CODEGEN_GCSTRATEGY_H
 
 #include "llvm/ADT/Optional.h"
+#include "llvm/IR/Value.h"
+#include "llvm/IR/Function.h"
+#include "llvm/IR/Module.h"
 #include "llvm/CodeGen/GCMetadata.h"
-#include "llvm/CodeGen/MachineFunction.h"
+#include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/Registry.h"
 #include <string>
 
 namespace llvm {
+  class MachineFunction;
+  
   /// GCStrategy describes a garbage collector algorithm's code generation
   /// requirements, and provides overridable hooks for those needs which cannot
-  /// be abstractly described.  GCStrategy objects currently must be looked up
-  /// through the GCModuleInfo analysis pass.  They are owned by the analysis
-  /// pass and recreated every time that pass is invalidated.
+  /// be abstractly described.  GCStrategy objects must be looked up through
+  /// the Function.  The objects themselves are owned by the Context and must
+  /// be immutable.
   class GCStrategy {
   private:
     std::string Name;
-    friend class GCModuleInfo;
+    friend class LLVMContextImpl;
     
   protected:
     bool UseStatepoints;       /// Uses gc.statepoints as opposed to gc.roots,

include/llvm/IR/Function.h

 namespace llvm {
 
 class FunctionType;
+class GCStrategy;  
 class LLVMContext;
 
 // Traits for intrusive list of basic blocks...

   void setGC(const char *Str);
   void clearGC();
 
+  /// Returns the GCStrategy associated with the specified garbage collector
+  /// algorithm or nullptr if one is not set.
+  GCStrategy *getGCStrategy() const;
+
   /// @brief adds the attribute to the list of attributes.
   void addAttribute(unsigned i, Attribute::AttrKind attr);
 

lib/CodeGen/GCMetadata.cpp

   initializeGCModuleInfoPass(*PassRegistry::getPassRegistry());
 }
 
-GCStrategy *GCModuleInfo::getOrCreateStrategy(const Module *M,
-                                              const std::string &Name) {
-  strategy_map_type::iterator NMI = StrategyMap.find(Name);
-  if (NMI != StrategyMap.end())
-    return NMI->getValue();
-  
-  for (GCRegistry::iterator I = GCRegistry::begin(),
-                            E = GCRegistry::end(); I != E; ++I) {
-    if (Name == I->getName()) {
-      std::unique_ptr<GCStrategy> S = I->instantiate();
-      S->Name = Name;
-      StrategyMap[Name] = S.get();
-      StrategyList.push_back(std::move(S));
-      return StrategyList.back().get();
-    }
-  }
- 
-  dbgs() << "unsupported GC: " << Name << "\n";
-  llvm_unreachable(nullptr);
-}
-
 GCFunctionInfo &GCModuleInfo::getFunctionInfo(const Function &F) {
   assert(!F.isDeclaration() && "Can only get GCFunctionInfo for a definition!");
   assert(F.hasGC());

   if (I != FInfoMap.end())
     return *I->second;
   
-  GCStrategy *S = getOrCreateStrategy(F.getParent(), F.getGC());
+  GCStrategy *S = F.getGCStrategy();
+  if (!S) {
+    dbgs() << "unsupported GC: " << F.getGC() << "\n";
+    llvm_unreachable(nullptr);
+  }
+  // Save the fact this strategy is associated with this module.  Note that
+  // these are non-owning references, the GCStrategy remains owned by the
+  // Context. 
+  StrategyList.push_back(S);
   Functions.push_back(make_unique<GCFunctionInfo>(F, *S));
   GCFunctionInfo *GFI = Functions.back().get();
   FInfoMap[&F] = GFI;

 void GCModuleInfo::clear() {
   Functions.clear();
   FInfoMap.clear();
-  StrategyMap.clear();
   StrategyList.clear();
 }
 

lib/IR/Function.cpp

   }
 }
 
+GCStrategy *Function::getGCStrategy() const {
+  // Lookup the GCStrategy (which is owned by the Context), given the name of
+  // the GC in question.
+  const char *Name = getGC();
+  return getContext().pImpl->getGCStrategy(Name);
+}
+
 /// copyAttributesFrom - copy all additional attributes (those not needed to
 /// create a Function) from the Function Src to this one.
 void Function::copyAttributesFrom(const GlobalValue *Src) {

lib/IR/LLVMContextImpl.h

 class DiagnosticInfoOptimizationRemark;
 class DiagnosticInfoOptimizationRemarkMissed;
 class DiagnosticInfoOptimizationRemarkAnalysis;
+class GCStrategy;
 class LLVMContext;
 class Type;
 class Value;

 
   int getOrAddScopeRecordIdxEntry(MDNode *N, int ExistingIdx);
   int getOrAddScopeInlinedAtIdxEntry(MDNode *Scope, MDNode *IA,int ExistingIdx);
+
+  /// An owning list of all GCStrategies which have been created
+  SmallVector<std::unique_ptr<GCStrategy>, 1> GCStrategyList;
+  /// A helper map to speedup lookups into the above list
+  StringMap<GCStrategy*> GCStrategyMap;
+
+  /// Lookup the GCStrategy object associated with the given gc name.  If one
+  /// can't be found, returns nullptr.  The lifetime of the returned objects
+  /// is dictated by the lifetime of the associated context.  No caller should
+  /// attempt to delete the returned objects.
+  GCStrategy *getGCStrategy(const StringRef Name);
   
   LLVMContextImpl(LLVMContext &C);
   ~LLVMContextImpl();

lib/IR/LLVMContextImpl.cpp

 
 #include "LLVMContextImpl.h"
 #include "llvm/ADT/STLExtras.h"
+#include "llvm/CodeGen/GCStrategy.h"
 #include "llvm/IR/Attributes.h"
 #include "llvm/IR/DiagnosticInfo.h"
 #include "llvm/IR/Module.h"

 void GetElementPtrConstantExpr::anchor() { }
 
 void CompareConstantExpr::anchor() { }
+
+GCStrategy *LLVMContextImpl::getGCStrategy(const StringRef Name) {
+  // TODO: Arguably, just doing a linear search would be faster for small N
+  auto NMI = GCStrategyMap.find(Name);
+  if (NMI != GCStrategyMap.end())
+    return NMI->getValue();
+  
+  for (GCRegistry::iterator I = GCRegistry::begin(),
+         E = GCRegistry::end(); I != E; ++I) {
+    if (Name == I->getName()) {
+      std::unique_ptr<GCStrategy> S = I->instantiate();
+      S->Name = Name;
+      GCStrategyMap[Name] = S.get();
+      GCStrategyList.push_back(std::move(S));
+      return GCStrategyList.back().get();
+    }
+  }
+
+  // No GCStrategy found for that name, error reporting is the job of our
+  // callers. 
+  return nullptr;
+}
+
+