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

[Nikita Popov via Phabricator via llvm-commits]

nikic 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())
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94743