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

[Stefan Stipanovic via Phabricator via llvm-commits]

sstefan1 marked 2 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;
+
----------------
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.


================
Comment at: llvm/lib/Transforms/IPO/Attributor.cpp:3048
+        IsAssumedSideEffectFree = true;
+        return indicateOptimisticFixpoint();
+      }
----------------
sstefan1 wrote:
> jdoerfert wrote:
> > sstefan1 wrote:
> > > jdoerfert wrote:
> > > > This is assumed information, correct? If so, we cannot indicate a fixpoint and we need to record a dependence if we use the information.
> > > Correct. Interestingly, though, it doesn't work without indicating fixpoint.
> > "Doesn't work" is a symptom not an explanation ;)
> > 
> > We cannot indicate an optimistic fixpoint based on assumed information. If the tests fail it needs to be investigated why. See my other comment though.
> This was a quick comment. Didn't have time to look into it more carefully. I'll take a closer look tomorrow.
H2S stuff needed to be checked before the if statement below. In latter updates `IsAssumedSideEffectFree` is true but `isAssumedSideEffectFree()` returns false causing update to return `CHANGED`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72562