[PATCH] D71959: [PassManagerBuilder] Remove global extension when a plugin is unloaded

[Elia Geretto via Phabricator via llvm-commits]

EliaGeretto added inline comments.


================
Comment at: llvm/include/llvm/Transforms/IPO/PassManagerBuilder.h:200
+                                              ExtensionFn Fn);
+  static void removeGlobalExtension(GlobalExtensionID ExtensionID);
   void addExtension(ExtensionPointTy Ty, ExtensionFn Fn);
----------------
grosser wrote:
> While the documentation is sparse already, I feel it would be helpful to document the newly introduced GlobalExtensionID.
I will do this in the next revision.


================
Comment at: llvm/lib/Transforms/IPO/PassManagerBuilder.cpp:211
+    PassManagerBuilder::GlobalExtensionID ExtensionID) {
+  if (GlobalExtensionsNotEmpty()) {
+    GlobalExtensions->erase(
----------------
grosser wrote:
> Why do we check if GlobalExtensions is not empty? Should this condition not always be true assuming removeGlobalExtension was called with a valid and registered ExtensionID? If this is the case, would it not make more sense to 'assert' in case this invariant is broken rather than silently ignoring the request to remove an invalid ExtensionID?
> 
> Also, rather than checking if GlobalExtensionsNotEmpty holds, should we not check that the specific ExtensionID is in the GlobalExtensions vector?
It was my mistake to use the `GlobalExtensionsNotEmpty` function. I am relying on the fact that `isConstructed` will return `false` if `GlobalExtensions` has already been destructed using `destroy`. In that case, nothing should be done; this is the case if LLVM is compiled without Polly. `removeGlobalExtension` is meant to do something only if `GlobalExtensions` was not destroyed yet; this is instead the case when compiling with Polly. I will use `isConstructed` directly and add a comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71959