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

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

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...

>>> 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.



================
Comment at: lib/Transforms/IPO/FunctionAttrs.cpp:1441
+  SmallVector<std::pair<const BasicBlock *, const BasicBlock *>> Backedges;
+  FindFunctionBackedges(F, Backedges);
+  if (!Backedges.empty())
----------------
nikic wrote:
> fhahn wrote:
> >  We do not really need to find all back edges, wouldn't it be sufficient to just traverse the CFG and exit once we found a cycle?
> I don't think FindFunctionBackedges is expensive enough to warrant reimplementing it here. Note that just keeping a visited set doesn't cut it, as that would detect any join point. You need to perform a DFS search, and doing that non-recursively is ugly enough that I'd rather not repeat it.
I ran some tests to check the compile-time impact. For this patch as-is: https://llvm-compile-time-tracker.com/compare.php?from=17fb21f875f4aaf6ad2cf9499cb75d76588167f2&to=335a834be8fe9a571fdbeea1262989c704ba91cf&stat=instructions There is a 0.1% regression. However, if I comment out the `setWillReturn` call, I get: http://llvm-compile-time-tracker.com/compare.php?from=17fb21f875f4aaf6ad2cf9499cb75d76588167f2&to=1b9a75cff1543e5ca29dea0dd56b111312901d98&stat=instructions So it seems that the impact here is due to second-order effects. The `FindFunctionBackedges` call itself doesn't appear to be problematic.


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

https://reviews.llvm.org/D94633