[PATCH] D39388: [RS4GC] Strip off invariant.start because memory locations arent invariant
Daniel Neilson via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Nov 1 10:18:13 PDT 2017
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
More information about the llvm-commits
mailing list