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

Nikita Popov via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Dec 3 11:54:29 PST 2021


nikic added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3741
+      if (Scan->mayHaveSideEffects())
+        return false;
+    }
----------------
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.


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

https://reviews.llvm.org/D109917



More information about the llvm-commits mailing list