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

Anna Thomas via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Nov 1 10:40:57 PDT 2017


anna added inline comments.


================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:131
       // one function changed, we know that the precondition is satisfied.
       stripNonValidAttributesAndMetadata(M);
     }
----------------
dneilson wrote:
> Note that this method is called if *any* function in the entire module is changed. This is also the only call of this method.
yes, that is what we want, we have to be conservative for correctness. We do not know at which stage RS4GC is done in the pipeline and we cannot rely on that information.

 If we have one more stage of inlining after RS4GC, followed by other optimizations, there is a possibility of the metadata being incorrect. Just as an example, consider invariant.start at the end of function F and F has no calls within it. So, RS4GC does nothing for this function. If we didn't strip off invariant.start and then inlined F into G after RS4GC, we can still face this same problem.

That is why we remove all incorrect metadata, attributes and instructions from all functions.




================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:2388
 
 void RewriteStatepointsForGC::stripNonValidAttributesAndMetadataFromBody(Function &F) {
   if (F.empty())
----------------
dneilson wrote:
> 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.
Regarding the name, I'll change it to `stripNonValidDataFromBody`.  

Regarding the second concern, I've addressed it in the first comment response. We actually need to remove the invariant.start for correctness from *every* function.


================
Comment at: lib/Transforms/Scalar/RewriteStatepointsForGC.cpp:2478
   for (Function &F : M)
     stripNonValidAttributesAndMetadataFromBody(F);
 }
----------------
dneilson wrote:
> Note: Indiscriminately called for every function in the module.
yes, that''s what we want. 


https://reviews.llvm.org/D39388





More information about the llvm-commits mailing list