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

[Denis Antrushin via Phabricator via llvm-commits]

dantrushin added a comment.

BTW, there is no need to create new revision to reflect trunk changes. You could simply rebase your local branch and then update diffs for the existing review. This works well and is common practice.

As for review itself.
First, I don't think that GCStrategy cache really belongs here. If I want to use GCStrategy say, in Verifier or in other Transform passes, should I add yet another GCStrategy cache everywhere? I don't think so.
The question is where to implement it. There was an attempt to move it to LLVMContext (see D6811 <https://reviews.llvm.org/D6811>), but it was reverted later (commit 56a03938f7a536dc2cef3c42ed67b3f218a1c758).
As hinted in mentioned commit we can create an IR analysis pass for that (which would duplicate CodeGen's `GCModuleInfo` to some extent, but I think we can live with that).
There even was arguing if GCStrategy are singletons or not (see discussions in D100559 <https://reviews.llvm.org/D100559>). @reames  thinks they are and I'm leaning to agree with him. But if they don't then bare `llvm::getGCStrategy()` would suffice.

Second, a bunch of static functions with long parameter lists start looking ridiculously. Local  class which keeps per-function state would look cleaner IMHO, but that can be done separately from this change.



================
Comment at: llvm/docs/Statepoints.rst:582
 that the ``statepoint-example`` GC strategy uses to distinguish references from
 non references.  The pass assumes that all addrspace(1) pointers are non-integral
+pointer types.  Address space 1 is not globally reserved for this purpose.  This
----------------
This is not true anymore ('The pass assumes that all addrspace(1) pointers are non-integral pointer types')? Perhaps simply delete this sentence?



================
Comment at: llvm/docs/Statepoints.rst:586
+in the ``statepoint-example`` and ``coreclr`` strategies, however custom
+strategies don't have to follow this convention.
 
----------------
This seems to repeat what was said in first sentence of paragraph.

But English is not my native language, so I'd leave review of this to native speakers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141110