[PATCH] D39388: [RS4GC] Strip off invariant.start because memory locations arent invariant

[Daniel Neilson via Phabricator via llvm-commits]

dneilson added inline comments.


================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:131
       // one function changed, we know that the precondition is satisfied.
       stripNonValidAttributesAndMetadata(M);
     }
----------------
Note that this method is called if *any* function in the entire module is changed. This is also the only call of this method.


================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:2388
 
 void RewriteStatepointsForGC::stripNonValidAttributesAndMetadataFromBody(Function &F) {
   if (F.empty())
----------------
It seems really odd & counterintuitive to me that a method that purports to strip attributes & metadata will now also be removing instructions. I'd never expect that from the name of the method.

I'm not a fan of introducing another loop over every instruction, but also not a fan of instructions being removed in a method with a name like this. Not sure how to resolve that.

Also, due to the way that this method is invoked the result of putting the invariant.start removal in this method is that if RS4GC changes any function in the module (even if it's only one) then we will remove the invariant.start/end pairs from *every* function in the module. That doesn't seem correct to me.


================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:2478
   for (Function &F : M)
     stripNonValidAttributesAndMetadataFromBody(F);
 }
----------------
Note: Indiscriminately called for every function in the module.


https://reviews.llvm.org/D39388