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

[Anna Thomas via Phabricator via llvm-commits]

anna added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3741
+      if (Scan->mayHaveSideEffects())
+        return false;
+    }
----------------
nikic wrote:
> anna wrote:
> > 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?
> > 
> If `wouldInstructionBeTriviallyDeadOnUnusedPaths()` is true, then you are basically promising that `call @A` cannot throw, or at least we're allowed to pretend it can't. Otherwise it would clearly be illegal to sink the call past control flow in any case.
> 
> Now, do we care about whether `call @B` throws? If we sink `call @A` past a call that throws or diverges, then side-effects from `call @A` may not occur, but the whole premise of the optimization is that they do not matter. You are already sinking the call past explicit control flow into a different block, so I don't see why sinking it past implicit control flow would be relevant.
> If wouldInstructionBeTriviallyDeadOnUnusedPaths() is true, then you are basically promising that call @A cannot throw, or at least we're allowed to pretend it can't. Otherwise it would clearly be illegal to sink the call past control flow in any case.
Yes, that's true. I missed that while writing the example.

> If we sink call @A past a call that throws or diverges, then side-effects from call @A may not occur, but the whole premise of the optimization is that they do not matter.
I had added the more general "no side-effect instructions in between" constraint so that we do not change the observable behaviour of the program. By sinking it past a throwable call, we are changing the observable behaviour of the program (but your point below clarified the position for me).

> You are already sinking the call past explicit control flow into a different block, so I don't see why sinking it past implicit control flow would be relevant.
Good point. I think with this logic, if we sunk some "allocation calls" to a block that wasn't executed at runtime, we are already changing the observable behaviour of the program and that is fine, since the allocation isn't used. 

Thinking through couple of examples:

Example 1:
```
  %x = call @A(i32 %d) <-- side-effecting because it writes to memory
  %y = call @A(i32 %d2)
...
  bb:
   use (%x)
   use(%y)
```
In this case, the memory-write variant is enough to make sure that we do not reorder the side-effecting calls in the bb block. After the transform we will (iteratively) sink both calls to `@A` to the use block without reordering the instructions

Example 2:
```
  %x = call @A(i32 %d) <-- side-effecting because it will not return
  %y = call @A(i32 %d2)
...
  bb:
   use (%x)
   use(%y)
```
In Example2, we already fail at `wouldInstructionBeTriviallyDeadOnUnusedPaths` since instruction does not return. Nothing will be sunk to use.

The last one is the "sinking past a throwable call":

```
   %x = call @A(i32 %d) <-- side-effecting because it writes to memory
   call void @B() <-- can throw (i.e. not marked nounwind)
   ..
bb:
   use (%x)
```

This is same as the case with explicit control flow:
```
  %x = call @A(i32 %d) <-- side-effecting because it writes to memory
  br i1 %false, label %fail, label %pass

fail:
  use(%x)

pass:
 ...
```

In short, I think you've convinced me with the reasoning for just writes to memory constraint. I will add a detailed comment explaining the reasoning (especially since it wasn't clear to me at first).





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

https://reviews.llvm.org/D109917