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

[Anna Thomas via Phabricator via llvm-commits]

anna added a comment.

In D109917#3165100 <https://reviews.llvm.org/D109917#3165100>, @nikic wrote:

> Do you have a use case for this apart from sinking allocations? If not, I would recommend restricting this to allocations only, because the generalized reasoning seems pretty subtle to me, and I'm not sure in which cases (apart from allocations) it would help.

To be honest, I ended up writing a sinking allocations test support, just so that we have this code upstream :) 
In our downstream use case, we have support for additional "trivially dead" instructions in java world, such as object.hashCode and identityHashCode. These modify state, so they are not `readnone`, but they can be sunk to uses (which is beneficial especially if use is on cold path).



================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3741
+      if (Scan->mayHaveSideEffects())
+        return false;
+    }
----------------
nikic wrote:
> I find the reasoning in terms of mayHaveSideEffects() in this patch confusing. A "side effect" in LLVM can either be a memory write, unwinding or divergence. I think the side effects you have in mind here are only of the "memory write" variety. I'd prefer if this code was more explicit about the actual correctness requirements.
I did mean instructions that are side-effecting, not just the subset of "memory write". For an allocation, it may be enough to worry about "memory writes" only, but we're looking at the entire set of Instructions that are trivially dead on unused paths (see main comment about downstream use cases for example).

```
bool Instruction::mayHaveSideEffects() const {
    return mayWriteToMemory() || mayThrow() || !willReturn();
  }
```

Consider this:
```
  %x = call @A() <-- mayThrow
  %y = call @B() <-- mayThrow`
...

BB:
  use (%x)
```
If the original behaviour of the program was such that call to `@A` can throw an exception and we can handle the exception and `call @B` also throws an exception, by sinking the `call @A` we have changed the order of exceptions. 
Or even the case if `call @B` does not return.

I am not sure why we are making it more general here by relaxing the restrictions?



================
Comment at: llvm/test/Transforms/InstCombine/sink_instruction.ll:235
+;
+  %A = call double @log(double %d)
+  br i1 %cond, label %if, label %else
----------------
nikic wrote:
> This test doesn't make sense to me, at least as a test for the new functionality. It would already fold beforehand because `@log()` is marked side-effect free. I think you were trying to test the isMathLibCallNoop() case, but I don't think it is relevant to your patch, because it requires constant arguments -- in which case the result will be constant folded anyway and the call removed.
Yes, you're right. It is not relevant. Makes sense to land separately. 
I do not have a way to showcase a test since there's no test which would trigger in this patch upstream. At least pretty much everything we already have triggers without this patch (either they are side-effect free or the instruction itself has no uses such as true guards and assumes).

Unfortunately, the only testcase which works with this patch is the alloc case, but there was a request to separate out from this patch because it looks slightly orthogonal (D114648). 






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

https://reviews.llvm.org/D109917