[PATCH] Ignore lifetime intrinsics in use list for MemCpyOptimizer.

[Hal Finkel]

----- Original Message -----
> From: "Nick Lewycky" <nlewycky at google.com>
> To: reviews+D5020+public+9b13ae0ef41b9337 at reviews.llvm.org
> Cc: "me+llvm" <me+llvm at luqman.ca>, "Hal Finkel" <hfinkel at anl.gov>, "Nick Lewycky" <nicholas at mxc.ca>, "Chandler
> Carruth" <chandlerc at gmail.com>, "Commit Messages and Patches for LLVM" <llvm-commits at cs.uiuc.edu>
> Sent: Monday, September 1, 2014 3:47:09 PM
> Subject: Re: [PATCH] Ignore lifetime intrinsics in use list for MemCpyOptimizer.
> 
> 
> 
> 
> On 1 September 2014 03:22, Chandler Carruth < chandlerc at gmail.com >
> wrote:
> 
> 
> Ahem, this probably shouldn't have gone in quite yet. ;] It has some
> bad bugs in it.
> 
> 
> Doh, sorry. Before commit I spent a while thinking about where
> lifetime intrinsics could be placed such that ignoring them would
> make the transformation invalid (ie., you start relying on a
> lifetime.end'd value where previously you didn't). I didn't actually
> review the implementation.
> 

So did I, and moreover, I did in theory review the implementation. I think, however, that I saw what I expected to see and not what was actually there :(

> 
> Thank you for taking care of this.

Seconded!

 -Hal

> 
> 
> 
> 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.
> 
> ================
> Comment at: lib/Transforms/Scalar/MemCpyOptimizer.cpp:682-685
> @@ -681,2 +681,6 @@
> 
> return false;
> + } else if (const IntrinsicInst *IT = dyn_cast<IntrinsicInst>(U)) {
> + if (IT->getIntrinsicID() != Intrinsic::lifetime_start &&
> + IT->getIntrinsicID() != Intrinsic::lifetime_end)
> + continue;
> } else if (U != C && U != cpy) {
> ----------------
> 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.
> 
> http://reviews.llvm.org/D5020
> 
> 
> 
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory