This is an archive of the discontinued LLVM Phabricator instance.

[RS4GC] Strip off invariant.start because memory locations arent invariant
ClosedPublic

Authored by anna on Oct 27 2017, 2:05 PM.

Details

Summary

Invariant.start on memory locations has the property that the memory
location is unchanging. However, this is not true in the face of
rewriting statepoints for GC.
Teach RS4GC about removing invariant.start so that optimizations after
RS4GC does not incorrect sink a load from the memory location past a
statepoint.

Added test showcasing the issue.

Diff Detail

Repository
rL LLVM

Event Timeline

anna created this revision.Oct 27 2017, 2:05 PM
dneilson edited edge metadata.Oct 31 2017, 8:16 AM

Is is possible to also remove the corresponding invariant.end calls to the removed invariant.start calls? This patch will leave orphaned calls to invariant.end sitting around in the IR that have no corresponding start.

anna added a comment.Oct 31 2017, 9:02 AM

Is is possible to also remove the corresponding invariant.end calls to the removed invariant.start calls? This patch will leave orphaned calls to invariant.end sitting around in the IR that have no corresponding start.

Yes, we could remove it (which was what I initially did), but I didn't see any benefit for doing so. It would be special casing just one type of use of invariant.start. As testcase points out, we can have any instruction using invariant.start, and we cannot just remove those uses, because those uses may have other uses...

Just to be clear: having orphaned invariant.end is perfectly valid IR. On the other hand, as an IR cleanup, that's a good thing to do, especially because invariant.end cannot have any uses. I'll update the patch with special casing invariant.end

anna updated this revision to Diff 120998.Oct 31 2017, 9:39 AM

addressed review comments.

anna added a comment.Nov 1 2017, 9:29 AM

just a note: We haven't yet added support to forward loads/stores across calls when the calls are dominated by invariant.start. However, that's a perfectly legal transform, and this patch makes sure we don't miscompile when the transformation is supported.

dneilson added inline comments.Nov 1 2017, 10:18 AM
lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
131 ↗(On Diff #120998)

Note that this method is called if *any* function in the entire module is changed. This is also the only call of this method.

2388 ↗(On Diff #120998)

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.

2478 ↗(On Diff #120998)

Note: Indiscriminately called for every function in the module.

anna added inline comments.Nov 1 2017, 10:40 AM
lib/Transforms/Scalar/RewriteStatepointsForGC.cpp
131 ↗(On Diff #120998)

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.

2388 ↗(On Diff #120998)

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.

2478 ↗(On Diff #120998)

yes, that''s what we want.

anna updated this revision to Diff 121305.Nov 2 2017, 7:45 AM

Updated function names per review comment. NFC wrt previous diff.

dneilson accepted this revision.Nov 2 2017, 8:55 AM
This revision is now accepted and ready to land.Nov 2 2017, 8:55 AM
This revision was automatically updated to reflect the committed changes.