[PATCH] D116200: [instcombine] Allow sinking of calls with known writes to uses

Sun Jan 2 09:05:16 PST 2022

reames added a comment.

In D116200#3208032 <https://reviews.llvm.org/D116200#3208032>, @nikic wrote:

> The logic here looks correct to me, but I'm still a bit uncomfortable with this addition: The core transform here is that if you have a potentially written pointer argument that is not read after the call, then it is safe to sink it. However, we currently don't have a way to indicate that this argument is not going to be read lateron, so this ends up doing some ad-hoc analysis that involves scanning all alloca users. It will be hard to meaningfully extend this, because we can't (or at least shouldn't) add sophisticated alias analysis to InstCombine. On the other hand, DSE could easily determine this as a matter of course, i.e. mark an argument as dead even if the call cannot be dropped entirely.

I do agree with your point about this being generalizable by DSE, but I want to highlight that the otherwise unused alloca case is an entirely real one.  If you have an out-param which is not used in a C/C++ function, this is exactly the form we get.

> That would be more involved though and I don't want perfect to be the enemy of good. Is there an immediate need for this? From D109917 <https://reviews.llvm.org/D109917> I got the impression that this was added purely for the sake of test coverage, and not because of a performance issue encountered in the wild.

The original motivation for discovering the out-param example was test coverage, but the example is real.  I haven't tried to quantify how broadly it covers in real code bases, but it's certainly something I have seen in the wild.

Personally, I think this is justified.

I'll also note that while doing non-trivial AA in InstCombine is probably a no-no, a simple use walk and a dominance check is pretty consistent with the existing structure.  Doing just that would let us handle out-params which are only read on a success path.  That is a very very common idiom.  (i.e. picture a multiple condition if chain with one of them being a helper routine with an out-param)

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

https://reviews.llvm.org/D116200