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

Chandler Carruth chandlerc at gmail.com
Mon Sep 1 03:22:42 PDT 2014


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.

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






More information about the llvm-commits mailing list