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

[Denis Antrushin via Phabricator via llvm-commits]

dantrushin added a comment.

Please make a separate NFC review with GCStrategy changes. Besides being common llvm review practice it will allow downstream projects to handle this API change without breakage.
With separate reviews/commit we will have time to update our downstream strategies before RS4GC starts using new API.
If they come in single commit, it will immediately break all existing RS4GC users.



================
Comment at: llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:147
     // compiling code without a GCStrategy.
-    if (!shouldRewriteStatepointsIn(F))
+    if (!shouldRewriteStatepointsIn(F, GC.get()))
       continue;
----------------
I think getting GCStrategy from function should be moved inside this function. See comments below


================
Comment at: llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:153
     auto &TLI = FAM.getResult<TargetLibraryAnalysis>(F);
-    Changed |= runOnFunction(F, DT, TTI, TLI);
+    Changed |= runOnFunction(F, DT, TTI, TLI, GC.get());
   }
----------------
Same here. Move it inside and you don't need change header file


================
Comment at: llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:189
 
+      std::unique_ptr<GCStrategy> GC = findGCStrategy(F);
+
----------------
Ditto


================
Comment at: llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:335-337
+  if (auto IsManaged = GC->isGCManagedPointer(T))
+    return *IsManaged;
+  return true; // conservative - same as StatepointLowering
----------------
You can use optional::value_or here:
```
return GC->isGCManagerPointer(T).value_or(true);
```


================
Comment at: llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:3055
+
+  assert(Strategy && "GC strategy is required by function, but was not found");
+
----------------
Returning false and then asserting looks confusing, this will raise questions like what Strategy really is; is it different from F.getGC; etc.
I think it would be much mo readable to get GCStrategy from function right here and use it. 


================
Comment at: llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:3061
+  assert(Strategy->useStatepoints() &&
+         "GC strategy has useRS4GC but not useStatepoints set");
+  return true;
----------------
Moving this assert to `GCStrategy::useRS4GC()` looks more logical to me.


================
Comment at: llvm/lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:3072
+      return true;
+  }
+  return false;
----------------
You won't need this if you move getting GCStrategy inside `shouldRewriteStatepointsIn()`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D141110