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

[Nikita Popov via Phabricator via llvm-commits]

nikic added inline comments.


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


================
Comment at: llvm/test/Transforms/InstCombine/sink_instruction.ll:235
+;
+  %A = call double @log(double %d)
+  br i1 %cond, label %if, label %else
----------------
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.


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

https://reviews.llvm.org/D109917