[PATCH] D94106: [Local] Treat calls that may not return as being alive (WIP).

[Nikita Popov via Phabricator via llvm-commits]

nikic added a comment.

In D94106#2480348 <https://reviews.llvm.org/D94106#2480348>, @fhahn wrote:

> In D94106#2480229 <https://reviews.llvm.org/D94106#2480229>, @jdoerfert wrote:
>
>> In D94106#2480220 <https://reviews.llvm.org/D94106#2480220>, @nikic wrote:
>>
>>> Happy to see this issue finally fixed :)
>>>
>>> I think the general approach here should be more along the lines of infering WillReturn inside FuncAttrs, and then just checking the WillReturn attribute here. Especially for languages which have no forward-progress guarantee it is important that WillReturn actually gets inferred somewhere, otherwise intrinsics will be the only thing that set the attribute.
>>
>> Maybe if we turn on the attributor for a few selected attributes only?
>
> I think it would be good to approach this incrementally. So start with gradually making the behavior 'more' correct than it is at the moment, while not regressing on performance. Treating functions without side effects without `mustprogress` as 'alive' seems like a relatively safe thing to do as a start.
>
> We should also improve the inference, but to make progress it might be be desirable to not predicate one on the other to start with. I realize that having good-enough inference to start with is the more principled approach and I am not sure if the desire to make progress outweighs that in the case at hand.

I like incremental improvements, but I don't think this really qualifies. What you are doing here is not making the behavior more correct while not regressing performance -- you are making it more correct while not regressing performance //for languages with a forward-progress guarantee//. What's likely going to happen is that this lands because it works well enough for C++, and then the remaining and technically harder part gets forgotten...

It would be okay if you say that this optimization just isn't very valuable in the first place and we can switch it to only checking WillReturn, delay the actual inference of WillReturn in more places until a later time, and eat any regressions that fall out in the meantime because correctness is more important than performance. But I don't think that directly coupling it to MustProgress in this way is a good idea.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94106