This is an archive of the discontinued LLVM Phabricator instance.

Mark values as trivially dead when their only use is a start or end lifetime intrinsic.
ClosedPublic

Authored by zoecarver on May 4 2020, 12:28 PM.

Details

Summary

If the only use of a value is a start or end lifetime intrinsic then mark the intrinsic as trivially dead. This should allow for that value to then be removed as well.

The only case where we may run into problems is the following:

%a = alloca
%e = get_element_ptr %a
call void @llvm.lifetime.start.p0i8(%e)

The element will no longer be marked as live. But, in all cases (I think) we should be able to remove both the intrinsics and the GEP. So, I don't think this will be an issue.

Diff Detail

Event Timeline

zoecarver created this revision.May 4 2020, 12:28 PM
zoecarver retitled this revision from [NFC] Traverse function using dominator tree. to Mark values as trivially dead when their only use is a start or end lifetime intrinsic. .May 4 2020, 12:32 PM
zoecarver edited the summary of this revision. (Show Details)
zoecarver removed reviewers: jdoerfert, sstefan1.

Phab used the description and title from my last patch. Sorry about that.

jdoerfert added inline comments.May 4 2020, 6:35 PM
llvm/lib/Transforms/Utils/Local.cpp
413

Style: Capital letter for variables.

Initially, I would prefer to add lifetime markers to the set of droppable uses and ask if the operand has only droppable uses. On second thought, we might not want to do this here because it would aggressively remove assumes. This patch could have a similar effect for useful lifetime markers, e.g. if you cast a pointer twice, once for lifetime once for use. Now I thought first that is unlikely but one of the tests already shows exactly this behavior: https://godbolt.org/z/oRLsrp Removing these lifetime markers and the bitcast is arguably not what we want. Put the call to bar in a loop and it becomes clear.

Multiple ways forward here, two come to mind first: (1) restrict the operand to be an alloca (argument, global, ...) or, (2) strip some casts and geps in both directions and then apply (1).

WDYT?

llvm/test/Transforms/DeadStoreElimination/lifetime.ll
15

CHECK-NOT instead?

zoecarver marked 2 inline comments as done.May 4 2020, 7:16 PM

@jdoerfert thanks for the review!

llvm/lib/Transforms/Utils/Local.cpp
413

That's a fair point which I didn't think about. I'll start with (1) and we can go from there.

llvm/test/Transforms/DeadStoreElimination/lifetime.ll
15

I thought about it when I made the change. My logic was that a DSE test shouldn't be testing the removal of lifetime intrinsics. That being said, it is a DSE lifetime test so it could probably go either way. I'll use CHECK-NOT.

zoecarver updated this revision to Diff 262003.May 4 2020, 9:19 PM
zoecarver retitled this revision from Mark values as trivially dead when their only use is a start or end lifetime intrinsic. to Mark values as trivially dead when their only use is a start or end lifetime intrinsic..
zoecarver edited the summary of this revision. (Show Details)
  • CHECK-NOT in DSE/lifetime.ll.
  • Only mark alloc and global as dead.
zoecarver updated this revision to Diff 262004.May 4 2020, 9:20 PM
zoecarver marked an inline comment as done.
  • Diff from master
zoecarver updated this revision to Diff 262005.May 4 2020, 9:27 PM
  • Fix local variable spelling
  • Add global test and arg test
  • Remove unneeded changes to tests
Harbormaster failed remote builds in B55736: Diff 262004!
Harbormaster failed remote builds in B55737: Diff 262005!
jdoerfert accepted this revision.May 5 2020, 2:22 PM

Minor things that can be addressed prior to the commit. Otherwise, LGTM. Thx.

llvm/lib/Transforms/Utils/Local.cpp
413

auto *Arg =

no else after return.

-II->getArgOperand(1)
+Arg

llvm/test/Transforms/DCE/basic.ll
53

Add one negative test, bitcast + lifetime marker, with a comment.

This revision is now accepted and ready to land.May 5 2020, 2:22 PM
This revision was automatically updated to reflect the committed changes.
zoecarver reopened this revision.May 6 2020, 11:27 AM

I had to revert this because of all the test failures outside llvm. I'll submit it for re-review before re-committing.

This revision is now accepted and ready to land.May 6 2020, 11:27 AM
zoecarver updated this revision to Diff 262740.May 7 2020, 1:28 PM
  • Fix failing test

I'll re-commit tonight unless I hear otherwise.

This revision was automatically updated to reflect the committed changes.