[PATCH] D95288: [ValueTracking] Don't assume readonly function will return

Wed Jan 27 02:36:41 PST 2021

SjoerdMeijer added a comment.

In D95288#2524765 <https://reviews.llvm.org/D95288#2524765>, @fhahn wrote:

> In D95288#2520902 <https://reviews.llvm.org/D95288#2520902>, @SjoerdMeijer wrote:
>
>> In D95288#2520069 <https://reviews.llvm.org/D95288#2520069>, @jdoerfert wrote:
>>
>>>> Yeah, I agree that mustprogress, argmemonly and nosync should imply willreturn. However, FuncAttrs currently infers neither nosync nor argmemonly, so it would need those first. I think that the argmemonly check here was there to cover some common intrinsics like llvm.memcpy, but those are explicitly willreturn now.
>>>
>>> Post release branch we should start running a light Attributor, to get things like nosync, argmemonly, ... one by one.
>>
>> But just for my understanding, does this mean there’s a problem that we intend to fix after the release?
>
> I don't think there's a problem per-se. There will be a few cases where we now fail to remove dead calls, because `willreturn` is not being inferred, even though the function is actually `willreturn`. So far, we do not have any indication that this is causing performance problems in practice, but we should keep a close eye on the impact. (And there are still places in tree that assume functions will return, without `willreturn`, which potentially still hides some of the negative impact).
>
> Going forward, we now have greater motivation to improve attribute inference, because there's a tangible benefit, where none was before ('as long as `willreturn` is not useful, why infer it?'). I think what Johannes is eluding to is that now there is a bigger motivation to move towards using parts of the attributor to improve the inference.

Ok, makes sense, got it. Cheers.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95288