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

[Luqman Aden]

On Sep 1, 2014, at 5:06 PM, Hal Finkel <hfinkel at anl.gov> wrote:

> ----- 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 :(

Oh, whoops! I ended up mistakenly blowing away the change originally, I must’ve mis-transcribed it the second time round. Thank you!

> 
>> 
>> 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