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

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3718
+          (isStackStateMod && Scan->isFunctionStackStateModifying()))
+        return false;
+    }
----------------
reames wrote:
> 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;
> 
JFYI, my potential cases just described are definitely disallowed by the function comment in the header which describes trivial-deadness.  Unfortunately, so is malloc.  As such, that comment is definitely wrong, and needs updated.  :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109917