[PATCH] D72562: [Attributor][Fix] AAHeapToStack and AAIsDead connection

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2915
+      if (isMallocOrCallocLikeFn(I, TLI) || isFreeCall(I, TLI))
+        return ChangeStatus::CHANGED;
+
----------------
sstefan1 wrote:
> sstefan1 wrote:
> > jdoerfert wrote:
> > > sstefan1 wrote:
> > > > jdoerfert wrote:
> > > > > sstefan1 wrote:
> > > > > > jdoerfert wrote:
> > > > > > > Would it work the other way around, as well? Deleting them here not in H2S. If so it would remove the special case here and some code in H2S, right?
> > > > > > I had this in mind, but I don't think it would work because `isAssumedSideEffectFree()` will be false.
> > > > > I see. In that case, can you move the special logic into `isAssumedSideEffectFree` to cut down on the duplication and allow reuse later on?
> > > > I've given this some more thought. While it would indeed be nicer to delete H2S stuff here as well, I think that can't work. Consider this example from the tests:
> > > > 
> > > >   define void @nofree_arg_only(i8* %p1, i8* %p2) {
> > > >     tail call void @free(i8* %p2)
> > > >     tail call void @nofree_func(i8* %p1)
> > > >     ret void
> > > >   }
> > > > 
> > > > Here `free()` is neither nounwind nor readonly but would be considered side effect free because we made `isAssumedSideEffectFree` consider it that way. As a result, since it is void value, it is deleted. Although it shouldn't have been.
> > > > 
> > > > If you still think we can/should do it, let me know.
> > > I did confuse you by putting the comment here, sorry. The code I want to move into `isAssumedSideEffectFree` is the code below, not the code here. By moving the logic from `updateImpl` into `isAssumedSideEffectFree` we should be able to remove the code here, that is what I should have said.
> > > 
> > > 
> > > > Here free() is neither nounwind nor readonly but would be considered side effect free because we made isAssumedSideEffectFree consider it that way. 
> > > 
> > > I don't think this is what should happen. `isAssumedSideEffectFree` will ask H2S and H2S::isAssumedToBeDeleted() will return `false` which will cause the normal logic in `isAssumedSideEffectFree` to continue and conclude we don't know anything about `free` and we need to assume it has side effects.
> > > 
> > That was the case :). The quoted comment was explaining how things would go if I were to move the code from above into the `isAssumedSideEffectFree`.
> > 
> > I think that should work.
> > 
> Unfortunately, I have to go back to being against this idea as I've stumbled upon another problem...
> 
>   define void @test3() {
>     %1 = tail call noalias i8* @malloc(i64 4)
>     tail call void @no_sync_func(i8* %1)
>     tail call void @free(i8* %1)
>     ret void
>   }
> 
> If `isAssumedSideEffectFree()` returned true the malloc would end up having live users in `no_sync_func()`. We need to skip checking `areAllUsesAssumedDead` for H2S stuff. I'm not sure if sticking with this would be worth it in the end.
OK, but we have to avoid duplication of H2S related code (here the ismalloc/isfree query). Having AAIsDead delete the code also has some appeal to it.

What if we add a boolean that tracks if we replace the value and therefore do not need to look at the users. We would not need this change here as `isAssumedSideEffectFree` returns true then. We also don't need to delete in H2S anymore.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:4920
+    if(IsMalloc && llvm::any_of(MallocCalls, IsI))
+      return true;
+
----------------
jdoerfert wrote:
> ```
> if(IsMalloc)
>   return llvm::any_of(MallocCalls, IsI);
> ```
if it is a malloc we don't need to check the free's below, right?


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:4910
+    if (!getAssumed())
+      return false;
+
----------------
```
if (BadMallocCalls.count(I))
  return false;
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72562