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

[Stefan Stipanovic via Phabricator via llvm-commits]

sstefan1 marked 3 inline comments as done.
sstefan1 added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:2915
+      if (isMallocOrCallocLikeFn(I, TLI) || isFreeCall(I, TLI))
+        return ChangeStatus::CHANGED;
+
----------------
jdoerfert wrote:
> 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.
I've given this another try.

If we delegate the deletion to `AAIsDead` we need 2 runs of Attributor. As a result H2S does its job on the first run but only converts mallocs to allocas leaving malloc behind. On the second run, leftover malloc is again converted to alloca and then finally deleted leaving us with 2 allocas.  Which is of course not good. The only thing that can be removed from here is `isFreeCall` check as it has no uses to be replaced with `undef`.

If we still want avoid deleting stuff in H2S, perhaps H2S needs a rework?
If that is the case I can abandon this and start looking in that direction, if possible. Otherwise, I don't think this is doable...


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:4920
+    if(IsMalloc && llvm::any_of(MallocCalls, IsI))
+      return true;
+
----------------
jdoerfert wrote:
> 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?
Right. I'll change this to your suggestion from above.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:4910
+    if (!getAssumed())
+      return false;
+
----------------
jdoerfert wrote:
> ```
> if (BadMallocCalls.count(I))
>   return false;
> ```
This doesn't take frees into account.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72562