[PATCH] D94743: [Attributor][FIX] Do not delete non`-mustprogress` calls

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Transforms/IPO/AttributorAttributes.cpp:2888
+    const auto &MustProgressAA = A.getAndUpdateAAFor<AAMustProgress>(
+        *this, CallIRP, /* TrackDependence */ false);
+    if (!MustProgressAA.isAssumedMustProgress())
----------------
nikic wrote:
> jdoerfert wrote:
> > nikic wrote:
> > > Shouldn't this be checking willreturn rather than mustprogress?
> > Hm, willreturn is stronger, isn't it (it implies mustprogress IIRC)?
> > But here we also require no side-effects so they are probably equivalent.
> > What do you think?
> I think semantically, willreturn is the correct one here. mustprogress works because we assume that mustprogress + readonly = willreturn. However, it would be good to have that knowledge encoded only in one place (willreturn inference), rather than in multiple. Especially as it is not strictly speaking correct, if we wanted to be pedantic about atomic loads (we don't want to though :).
> 
> Generally, I think that the `mustprogress` attribute should only be placed by the frontend, and not derived. Only `willreturn` should be derived, and that's the property that analyses should be checking.
> I think semantically, willreturn is the correct one here. mustprogress works because we assume that mustprogress + readonly = willreturn.

I think I agree. Will change it.

> Generally, I think that the mustprogress attribute should only be placed by the frontend, and not derived. 

Hm, maybe. It's not that the `mustprogress` deduction in D94740 actually does much but what it does cannot be replaced by `willreturn`, I think. Take
```
static void foo() { ... }
void bar() __attribute__((mustprogress)) { foo() }
```
We can conclude that `foo` needs to make progress but it might not return/unwind at all.
Maybe I'm confused again.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94743