This is an archive of the discontinued LLVM Phabricator instance.

Ignore lifetime intrinsics in use list for MemCpyOptimizer.
ClosedPublic

Authored by luqmana on Aug 21 2014, 9:28 PM.

Details

Summary

Lifetime intrinsics were preventing various optimizations like turning memset+memcpy to just memset.

Diff Detail

Event Timeline

luqmana updated this revision to Diff 12825.Aug 21 2014, 9:28 PM
luqmana retitled this revision from to Ignore lifetime intrinsics in use list for MemCpyOptimizer..
luqmana updated this object.
luqmana edited the test plan for this revision. (Show Details)
luqmana set the repository for this revision to rL LLVM.
luqmana added a subscriber: Unknown Object (MLST).
hfinkel accepted this revision.Aug 26 2014, 11:16 PM
hfinkel added a reviewer: hfinkel.
hfinkel added a subscriber: hfinkel.

LGTM.

This revision is now accepted and ready to land.Aug 26 2014, 11:16 PM

Cool! Could someone please land this for me, I don't have commit access.

nicholas closed this revision.Aug 31 2014, 11:12 PM
nicholas edited edge metadata.

Committed revision 216865.

Ahem, this probably shouldn't have gone in quite yet. ;] It has some bad bugs in it. In the future, I would suggest always including a negative test (or ensuring there is an existing negative test) to make sure the transformation logic you intended to have is in fact there.

See below for the details. I've gone ahead and fixed this in ToT to unbreak things.

lib/Transforms/Scalar/MemCpyOptimizer.cpp
682–685

This is wrong in several ways.

First off, it is identifying *non* lifetime intrinsics here, not lifetime intrinsics. But *non* lifetime intrinsics definitely shouldn't be "continue". I think the logic of the if is just reversed.

The other problem is that falling through the if... actually just does exactly the same thing as the body of the if -- it continues. This is equivalent to just continuing for *all* intrinsics, lifetime or otherwise. That in turn can miscompile code if the intrinsics are actually using the pointer and are not either C or cpy as checked below.

I've substantially rewritten the patch in r216871 to address both of these points. Please review it to make sure it matches what you expected.