[PATCH] D141110: [RS4GC] Remove the hardcoded GC strategy names (v2)

[Denis Antrushin via Phabricator via llvm-commits]

dantrushin added inline comments.


================
Comment at: llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:3049
+/// function doesn't have a GC tag. The strategy is stored in the cache.
+static GCStrategy *findGCStrategy(Function &F, GCStrategyCache &cache) {
+  if (!F.hasGC())
----------------
reames wrote:
> I'm confused by your implementation here.  getGCStrategy uses the register of GCStrategy objects internally.  If calling that works, why do you need another caching layer here at all?
> 
> As a bit of history, the GC registery was initially in CodeGen and not available in the rest of LLVM due to some weird linkage problems.  I think - though I don't quite remember details, so check - that got fixed years ago.  
@reames: `llvm::getGCStrategy()` calls `GCStrategy::instantiate()` for every query (creating new instance).
CodeGen's `GCModuleInfo::getGCStrategy()` implements GC strategy cache on top of that. I guess, in attempt to have them as singletons (?).
Do you think we don't need it in IR?
I also found your previous attempt to move ownership of GCStrategies to LLVM Context which was reverted and you mentioned that it
makes sense to have them in Immutable Analysis pass. Was its only purpose solve that linkage problem?
If yes, we can drop this GCStrategy cache completely and call `llvm::getGCStrategy()`directly.

BTW, that linkage problem was fixed recently by Campbell.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D141110/new/

https://reviews.llvm.org/D141110