[PATCH] D155406: [MemCpyOpt] implement multi BB stack-move optimization

Wed Sep 13 13:32:10 PDT 2023

vitalybuka added a comment.

In D155406#4643864 <https://reviews.llvm.org/D155406#4643864>, @khei4 wrote:

> @vitalybuka Ah, sorry, I was confused and not sure what you said. Did you mean just stripping the src lifetime work?

Yes.

1. First you land the MemCpyOpt patch which remove lifetime from the preservers alloca. (assuming perf improvement from lifetime markers is smaller then from MemCpyOpt)
2. Then you land another patch with "correct" lifetimes. Looks like this is most tricky part so we can iterate here.

We have preliminary repro, which I will share by tomorrow.

But I still recommend to split the patch as above. Even with repro, I would not be surprised we hit another case and have to revert.

> In D155406#4641099 <https://reviews.llvm.org/D155406#4641099>, @vitalybuka wrote:
>
>> In D155406#4640773 <https://reviews.llvm.org/D155406#4640773>, @bgraur wrote:
>>
>>> @khei4 the last commit is causing again lots of false positive for tests executed under `asan`.
>>>
>>> We (the google compilers team) got several tests with no `asan` findings when built before rG3a1409f93da32bf626f76257e0aac71716f2f67e <https://reviews.llvm.org/rG3a1409f93da32bf626f76257e0aac71716f2f67e> that trigger `stack-use-after-scope` when built with this commit.
>>>
>>> Coming up with a reproducer is pretty time consuming, could you please revert this until one is available?
>>
>> reverted in efe8aa2e618122e8050af10cc5d6ad83f24ef557 <https://reviews.llvm.org/rGefe8aa2e618122e8050af10cc5d6ad83f24ef557>
>>
>> If this optimization is critical, maybe you can split this patch in two?
>>
>> 1. BB stack-move optimization which strips all lifetime markers from src alloca
>> 2. insert alloca on dst
>>
>> Then we can continue related/revert patch 2 as needed.

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155406/new/

https://reviews.llvm.org/D155406