[PATCH] D102113: [MemCpyOpt] Remove MemDepAnalysis-based implementation

Thu May 20 11:59:37 PDT 2021

asbirlea added a comment.

In D102113#2767998 <https://reviews.llvm.org/D102113#2767998>, @fhahn wrote:

> In D102113#2767220 <https://reviews.llvm.org/D102113#2767220>, @asbirlea wrote:
>
>> I believe the process is to additionally test a release after it's been cut, and potential issues that arise can be fixed and cherry-picked into that release. Flipping back the flag would probably not be a useful fix after 6+month, rather resolving the bug would be.
>> As an additional data point, we've had several internal releases since the flag was flipped and haven't encountered any issues. I'm not saying bugs are sure not to exist, just that there has been some fairly thorough testing done and I'd opt to fix forward rather than switch back to the MemDepAnalysis implementation.
>> If you have strong reasons to hold back another couple of month on this cleanup, that's an option too, I'm just outlining my reasoning for moving forward.
>
> My motivation to keep the code until the release was not so we can flip back the flag in the codebase; I agree we should just fix the bugs in the new version. I meant the flag may be helpful to end-users who cannot easily update after the release and could use the flag as workaround. It is all what-ifs at this point though :) If it makes planned upcoming work easier, removing the code now sounds good. If it is mostly a matter of removing unused code a bit earlier, I have a slight preference to keep it until the next release branch (I'm happy with the patch moving ahead, I mainly wanted to share another experience to consider)

Got it, thank you for explaining! Yes, I can get behind that. I don't have any imminent changes following this cleanup, so checking this in is not urgent. I'll leave @nikic answer if there's something on his end.

A somewhat hacky "work-around" if we're considering keeping this in the release, but also want an earlier clean-up is to land this now and once the release is cut have a pair of revert+recommit pushed in, out of which only the revert would be cherry-picked into the release. I'm not saying this is better, just an idea.

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

https://reviews.llvm.org/D102113