This is an archive of the discontinued LLVM Phabricator instance.

[CodeGen] Emit lifetime.end intrinsic after destructor call in landing pad
ClosedPublic

Authored by ahatanak on Mar 15 2016, 2:38 PM.

Details

Summary

This patch fixes a bug in CodeGen where lifetime.end intrinsics were not being inserted after destructor calls in landing pad blocks, which prevented StackColoring from merging stack slots for allocas because it couldn't tell their lifetime ranges were disjoint.

This patch changes the code in CodeGenFunction::EmitAutoVarCleanup to pass NormalAndEHCleanup instead of NormalCleanup to EHScopeStack::pushCleanup so that CallLifetimeEnd runs the cleanup code when a scope is exited using exceptional control flow too. I initially considered adding code to DestroyObject::Emit to emit lifetime.end after the destructor call, but letting CallLifetimeEnd emit lifetime.end seemed like a better approach.

This is the link to the discussion on llvm-dev:

http://lists.llvm.org/pipermail/llvm-dev/2016-March/096233.html

Diff Detail

Event Timeline

ahatanak updated this revision to Diff 50766.Mar 15 2016, 2:38 PM
ahatanak retitled this revision from to [CodeGen] Emit lifetime.end intrinsic after destructor call in landing pad.
ahatanak updated this object.
ahatanak added reviewers: rnk, rjmccall.
ahatanak added a subscriber: cfe-commits.
rjmccall edited edge metadata.Mar 15 2016, 4:49 PM

You should talk to Reid or someone else involved in MSVC-style EH support to ensure that they generate a reasonable code pattern for this.

You should also check that any back-end peepholes we have in place (null type infos to signify a call-terminate landingpad?) aren't disturbed by the lifetime intrinsics.

rnk edited edge metadata.Mar 16 2016, 9:44 AM

You should talk to Reid or someone else involved in MSVC-style EH support to ensure that they generate a reasonable code pattern for this.

I patched this in and verified it works. Clang makes a separate funclet for the lifetime end, which is suboptimal, but LLVM knows how to simplify it down to one. The stack coloring optimization in the backend doesn't fire yet, though. =/

Can you add a simple test for this? Make something similar to test/CodeGenCXX/microsoft-abi-eh-cleanups.cpp but with optimizations enabled.

I can add another RUN line to test/CodeGenCXX/microsoft-abi-eh-cleanups.cpp that is identical to the existing RUN line except that it has "-O3 -disable-llvm-optzns". I confirmed that the test still passes without any changes to the test itself.

Is that what you are looking for?

rnk added a comment.Mar 16 2016, 3:03 PM

I can add another RUN line to test/CodeGenCXX/microsoft-abi-eh-cleanups.cpp that is identical to the existing RUN line except that it has "-O3 -disable-llvm-optzns". I confirmed that the test still passes without any changes to the test itself.

Is that what you are looking for?

I assumed the test wouldn't pass with those flags, but yes, that works. Could you add the obvious CHECK lines for a simple cleanup though? There should be an invoke, two cleanuppads, one calls the dtor, the other ends the lifetime, they chain to each other.

ahatanak updated this revision to Diff 51376.Mar 22 2016, 6:58 PM
ahatanak edited edge metadata.

Added a test in test/CodeGenCXX/microsoft-abi-eh-cleanups.cpp to check expected patterns are emitted.

You should also check that any back-end peepholes we have in place (null type infos to signify a call-terminate landingpad?) aren't disturbed by the lifetime intrinsics.

I looked at the back-end passes that might be affected by the insertion of lifetime markers. I don't think inserting lifetime markers will have an adverse effect on any of the back-end code-generation.

ahatanak updated this revision to Diff 52110.Mar 30 2016, 1:17 PM

Add "REQUIRES: asserts" to test case to enable checking basic block names.

This revision was automatically updated to reflect the committed changes.