[PATCH] D94633: [FunctionAttrs] Infer willreturn for functions without loops

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

In D94633#2499063 <https://reviews.llvm.org/D94633#2499063>, @nikic wrote:

> In D94633#2498743 <https://reviews.llvm.org/D94633#2498743>, @jdoerfert wrote:
>
>> EDIT: I'm fine with this patch, FWIW.
>>
>> In D94633#2498642 <https://reviews.llvm.org/D94633#2498642>, @nikic wrote:
>>
>>> In D94633#2498420 <https://reviews.llvm.org/D94633#2498420>, @jdoerfert wrote:
>>>
>>>> I still think we should check to enable a light-attributor run in the beginning of the pipeline.
>>>
>>> I was hoping to get the willreturn-related bugs fixed in time for LLVM 12, so sticking to the existing infrastructure seems like the safer bet :) It probably makes more sense to enable the attributor after branching rather than before.
>>
>> Fair. As I said, we can enable it in a restricted, lightweight mode first. Could you remind me which bugs?
>
> D94106 <https://reviews.llvm.org/D94106> fixes the important one, though isGuaranteedToTransferExecutionToSuccessor is also buggy for languages without forward progress. Functions with infinite loops getting optimized away is the number one Rust miscompile by quite a margin...

Right. We should not delete non-mustprogress non-side-effect calls. Probably requires changes in a few more places.

>>>> The logic seems sound, could be improved since this does initialize the fixpoint computation with a pessimistic starting value. One comment below, the rest is OK for merging.
>>>
>>> I thought about this, but I don't think that we can use an optimistic fixpoint here without some much more sophisticated analysis. Just naively assuming "willreturn until proven otherwise" will incorrectly mark infinite recursion as willreturn:
>>>
>>>   ; Not willreturn.
>>>   define void @willreturn_recursion() {
>>>     tail call void @willreturn_recursion()
>>>     ret void
>>>   }
>>
>> So, I run this through the Attributor: https://opt.godbolt.org/z/TqsK86
>> Not what we were looking for, so here is the non-trivial version: https://opt.godbolt.org/z/rbeedM
>
> Just to make sure we're on the same page, both of those would be bugs in attributor right? The first one shouldn't be unreachable, and the second one shouldn't be willreturn, at least not without more information than is contained in that IR.

I agree for the first one. Mustprogress is missing for unreachable to correct. The second one is correct, or am I missing something?


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

https://reviews.llvm.org/D94633