[PATCH] D24854: [SROA] Drop lifetime.start/end intrinsics when they block promotion.

[Chandler Carruth via llvm-commits]

chandlerc added inline comments.


================
Comment at: lib/Transforms/Scalar/SROA.cpp:1738-1744
+    // lifetime.start and lifetime.end are promotable, but we avoid
+    // promoting them in some cases if asan is enabled.
     if (II->getIntrinsicID() != Intrinsic::lifetime_start &&
         II->getIntrinsicID() != Intrinsic::lifetime_end)
       return false;
+    if (II->getFunction()->hasFnAttribute(Attribute::SanitizeAddress))
+      return false;
----------------
This (and the bit below) seems like a really good change, and I can see why you might need to make it in order to make progress. But I think you'll need to split this out into a separate patch.

First, this needs its own test case. It can be pretty boring and just check that asan functions keep lifetime markers and avoid promoting.

But more importantly, this may have really significant knock-on effects for ASan. I just don't know. We should ask the ASan folks to test this and see what it does to things like the performance of ASan-instrumented code. My fear is that it may block *so much* promotion to registers that we actually make ASan unusably slow.

It may be necessary to instead do something much more fancy for ASan: we should instead always promote to a register but synthesize the necessary checks for ASan's use-after-scope directly without relying on memory. If this is something you're interested in playing with, I'm happy to chat with you and the ASan folks about how this might work. Otherwise, I'd suggest sending them this patch as a WIP and they can follow up when it gets to the top of their priorities.

See below for my suggestion on how to make progress in the interim though.


================
Comment at: lib/Transforms/Scalar/SROA.cpp:1954-1960
+    // lifetime.start and lifetime.end are promotable, but we avoid
+    // promoting them in some cases if asan is enabled.
     if (II->getIntrinsicID() != Intrinsic::lifetime_start &&
         II->getIntrinsicID() != Intrinsic::lifetime_end)
       return false;
+    if (II->getFunction()->hasFnAttribute(Attribute::SanitizeAddress))
+      return false;
----------------
(this is the other change I would group with the above)


================
Comment at: lib/Transforms/Scalar/SROA.cpp:2897-2901
+    bool IsWholeAlloca = NewBeginOffset == NewAllocaBeginOffset &&
+                         NewEndOffset == NewAllocaEndOffset;
+    if (!II.getFunction()->hasFnAttribute(Attribute::SanitizeAddress) &&
+        !IsWholeAlloca)
+      return true;
----------------
FWIW, I think this is sufficient to avoid regressing ASan while allowing promotion through partial lifetime markers. Is there something that breaks if you make this change and not the changes above? If so, I'd like to find a way to temporarily work around them as I think fully handling ASan + lifetime markers + promotable allocas is going to be a big project.


Repository:
  rL LLVM

https://reviews.llvm.org/D24854