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

[Anna Thomas via Phabricator via llvm-commits]

anna added a comment.

In D109917#3027817 <https://reviews.llvm.org/D109917#3027817>, @reames wrote:

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

For others following along: we plan to discuss offline. Will summarize result.

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

Unfortunately, the no-alias bit is required for the malloc, since the `Scan` instruction *starts* from the instruction we try to sink (i.e. the malloc call) and we fail at `mayWriteToMemory`. We can frame this as possibly a CT win and land it separately.



================
Comment at: llvm/lib/Transforms/InstCombine/InstructionCombining.cpp:3718
+          (isStackStateMod && Scan->isFunctionStackStateModifying()))
+        return false;
+    }
----------------
reames wrote:
> 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.  :)
I'm not sure if `trivial-deadness` is the property used in DSE for removing the malloc, but the case above is handled by DSE even if we have an externally arranged invariant through `malloc_library_counter` for example:

cat simple.ll
```
declare noalias i8* @malloc(i32) willreturn
declare i32 @malloc_library_counter()
define void @test20A() {
  %m = call i8* @malloc(i32 24) 
   %stat = call i32 @malloc_library_counter()
   store i8 0, i8* %m
   ret void
}

` opt -basic-aa -dse -S simple.ll` :

```
define void @test20A() {
  
  %stat = call i32 @malloc_library_counter()
  ret void
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109917