[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.

Skimming the code for this and the prerequisite patch, I'm a bit uncomfortable.  It really feels like you're trying to a) do several things in one change, and b) possibly abusing an interface for a purpose it wasn't intended.

I would suggest moving the no-alias-call bit to a separate patch as that seems unrelated to the core functionality.



================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3718
+          (isStackStateMod && Scan->isFunctionStackStateModifying()))
+        return false;
+    }
----------------
Hm, I am not sure here, but I think you should check for potential reads here as well.  

My reasoning is as follows.  While the instruction we're moving might be trivially dead if there are no users, it may *not* be trivially dead if there are users.  There may be some externally arranged invariant that (say) the result is stored with a volatile variable *if-and-only-if* the call actually has a side effect.  

As an example, consider the following:
a = malloc()
stat = malloc_library_counter()
if (c)
  free(a)

Basically, is it legal to sink the malloc in this case?  I legitimately don't know what the right answer is here.  

Another example might be:
a = malloc()
stat = malloc_library_counter()
if (c)
  (volatile i8*)g = a;



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109917