[PATCH] D109917: [InstCombine] Improve TryToSink for side-effecting calls that would be trivially dead

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

In D109917#3189682 <https://reviews.llvm.org/D109917#3189682>, @anna wrote:

> In D109917#3188468 <https://reviews.llvm.org/D109917#3188468>, @nikic wrote:
>
>> I see that you've added the writeonly argument handling to this patch. I would  prefer to split that off into a separate one, because it's not directly related. Even if it means that this lands without any upstream test changes at first.
>
> I don't think we generally do this. I will need to add either the allocations or the paramcase soon-ish after this. But I see your point (since the change as-is does not affect any upstream code). Will wait if @reames has any concerns.

I think we need to find a way to untangle this into something both reviewable and testable.

It occurs to me that the handling for writeonly calls is separable.  In addition to allowing sinking, we can also simply *delete* such calls.  As such, we should be able to write tests which exercise that logic on it's own without the remainder of the patch.

>> I think something that your reasoning on writeonly arguments may be missing is a capture by the call, in which case there might be an indirect read of the alloca later that is not covered by your analysis. Might not matter for the sinking as implemented here, but would make it illegal to drop the call.
>
> That's true. Note that the capture should be by another argument passed to the call since the called function is argmemonly.

I agree, the current code has this bug.

Returning to something discussed previously, I would feel more comfortable if the side effect reasoning to this patch was initially restricted to memory writes.  We can come back and generalize (the downstream case can be supported), but doing that in a separate change in case we miss something would make me more comfortable.



================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:508
+    // where the alloca is used only by lifetime markers and the call itself.
+    auto canCallBeDeleted = [Call]() {
+      auto *F = Call->getCalledFunction();
----------------
Please pull this out as a static function.  This is too long for a lambda.  


================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:513
+      // Are the arguments writeonly?
+      if (!llvm::all_of(F->args(), [](Argument &A) {
+            return A.hasAttribute(Attribute::WriteOnly);
----------------
This is too strict.  You can have other arguments provided they don't also write.

It's also incorrect as you allow a writeonly param to an unrelated global which we can't remove.  

What you want is to identify the writeonly params, and then check the uses in the search below correspond to them.  You must prove that a) the alloca only reaches no-capture uses on the call, and b) that all writeonly params correspond to uses of the alloca.  




================
Comment at: llvm/lib/Transforms/Utils/Local.cpp:541
+      }
+      return true;
+    };
----------------
For future exploration: It really feels like the property here of "this is never read from" is something we should be able to compute and cache much more broadly.  For instance, DSE will naturally find such cases, but not be able to sink them.  I don't really know what to do about that, but it sure feels like we should be able to do something.  


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

https://reviews.llvm.org/D109917