[GC][NFC] Make getGCStrategy by name available in IR

[mkazantsev]

Summary

We might want to use info from GC strategy in middle end analysis.
The motivation for this is provided in D99135: we may want to ask
a GC if it's going to work with a given pointer (currently this code
makes naive check by the method name).

[reames]

(Not reviewing until issue raised on previous patch addressed, please request review when that one is resolved.)

[mkazantsev]

I didn't receive any complains about linkage problems in previous patch, so re-requesting review.

I also tried dynamic linkage build on this one and it worked for me.

[reames]

I don't think the interface on this is quite right. You have the new utility exposing a new instance of the GCStrategy object. I would expect each GCStrategy to be a singleton. (i.e. having two "statepoint-example" GCStrategy instances would seem surprising.)

I think you need to move the ownership of GCStrategy objects from GCMetadata to LLVMContext, and then cache the lookup. You want to leave GCStrategyList in place, but probably make it non-owning.

[mkazantsev]

What's the problem of having multiple instances of the same GC strategy? Currently, each strategy has only const methods, so it doesn't matter which instance we are using. Even if we make them mutable, isn't it possible that we are willing to get some default strategy and then configure it, e.g., depending on a platform? In that case, having different instances is even useful.

[reames]

In D100559#2765430, @mkazantsev wrote:

What's the problem of having multiple instances of the same GC strategy? Currently, each strategy has only const methods, so it doesn't matter which instance we are using. Even if we make them mutable, isn't it possible that we are willing to get some default strategy and then configure it, e.g., depending on a platform? In that case, having different instances is even useful.

These are currently singleton objects; I'd like to preserve that. It would be reasonable for a custom strategy to e.g. cache computation in mutable state, and having two or more copies breaks that reasonable implementation in hard to predict ways.

If you think this is too large an ask implementation wise, say so. I'm not strongly tied to it, it's simply a mild preference.

[mkazantsev]

Why are they singleton objects now? In current code, they are instatiated as many times as getGCStrategy is called, and it's called once for every function in module from GCFunctionInfo &GCModuleInfo::getFunctionInfo(const Function &F) (if I'm reading this correctly). Am I missing something here?

[reames]

In D100559#2767733, @mkazantsev wrote:

Why are they singleton objects now? In current code, they are instatiated as many times as getGCStrategy is called, and it's called once for every function in module from GCFunctionInfo &GCModuleInfo::getFunctionInfo(const Function &F) (if I'm reading this correctly). Am I missing something here?

You are missing something.

GCModuleInfo::getGCStrategy maintains a map from name to GCStrategy object. If you look up the same name repeatedly (e.g. for every function), you get the same GCStrategy object. There is currently one object per GC name.

[mkazantsev]

So it's not a singleton, it's something that is cached inside of GCModuleInfo. If you have 2 different instances of GCModuleInfo, you will have two different GCStrategies with same name. And this patch preserves this behavior.

You can as well say that SCEV constant 1 of type i32 is a singleton, but it's not: it is something that is cached within a particular instance of ScalarEvolution.

Even if this thing was a singleton, I don't believe it's important for any practical purpose.

[reames]

Max, I'd still prefer the singleton, but frankly, I don't care enough to keep discussing this. Land what we've got; we can refactor at some point in the future.

llvm/include/llvm/IR/GCStrategy.h

 /// static GCRegistry::Add<CustomGC> X("custom-name",
 ///        "my custom supper fancy gc strategy");
 ///
 /// Note that to use a custom GCMetadataPrinter w/gc.roots, you must also
 /// register your GCMetadataPrinter subclass with the
 /// GCMetadataPrinterRegistery as well.
 using GCRegistry = Registry<GCStrategy>;
 
+/// Lookup the GCStrategy object associated with the given gc name.
+std::unique_ptr<GCStrategy> getGCStrategy(const StringRef Name);
+
 } // end namespace llvm
 
 #endif // LLVM_IR_GCSTRATEGY_H

llvm/lib/CodeGen/GCMetadata.cpp

 }
 
 GCStrategy *GCModuleInfo::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 (auto& Entry : GCRegistry::entries()) {
-    if (Name == Entry.getName()) {
-      std::unique_ptr<GCStrategy> S = Entry.instantiate();
-      S->Name = std::string(Name);
-      GCStrategyMap[Name] = S.get();
-      GCStrategyList.push_back(std::move(S));
-      return GCStrategyList.back().get();
-    }
-  }
-
-  if (GCRegistry::begin() == GCRegistry::end()) {
-    // In normal operation, the registry should not be empty.  There should
-    // be the builtin GCs if nothing else.  The most likely scenario here is
-    // that we got here without running the initializers used by the Registry
-    // itself and it's registration mechanism.
-    const std::string error = ("unsupported GC: " + Name).str() +
-      " (did you remember to link and initialize the CodeGen library?)";
-    report_fatal_error(error);
-  } else
-    report_fatal_error(std::string("unsupported GC: ") + Name);
-}
+  std::unique_ptr<GCStrategy> S = llvm::getGCStrategy(Name);

llvm/lib/IR/GCStrategy.cpp

 
 #include "llvm/IR/GCStrategy.h"
 
 using namespace llvm;
 
 LLVM_INSTANTIATE_REGISTRY(GCRegistry)
 
 GCStrategy::GCStrategy() = default;
+
+std::unique_ptr<GCStrategy> llvm::getGCStrategy(const StringRef Name) {
+  for (auto &S : GCRegistry::entries())
+    if (S.getName() == Name)
+      return S.instantiate();
+
+  if (GCRegistry::begin() == GCRegistry::end()) {
+    // In normal operation, the registry should not be empty.  There should
+    // be the builtin GCs if nothing else.  The most likely scenario here is
+    // that we got here without running the initializers used by the Registry
+    // itself and it's registration mechanism.
+    const std::string error =
+        std::string("unsupported GC: ") + Name.str() +
+        " (did you remember to link and initialize the library?)";
+    report_fatal_error(error);
+  } else
+    report_fatal_error(std::string("unsupported GC: ") + Name.str());
+}