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

[Nick Lewycky]

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.

Thank you for taking care of this.

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
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140901/23407cfc/attachment.html>