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

[Florian Hahn via Phabricator via llvm-commits]

fhahn added a comment.

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.



================
Comment at: llvm/include/llvm/IR/InstrTypes.h:1763
+    return hasFnAttr(Attribute::WillReturn) ||
+           hasFnAttr(Attribute::MustProgress);
+  }
----------------
jdoerfert wrote:
> jdoerfert wrote:
> > nikic wrote:
> > > This isn't right, MustProgress does not guarantee that the function returns. I'd suggest to take a look at the logic in isGuaranteedToTransferExecutionToSuccessor() (which should be replaced as well).
> > `willreturn || (mustprogress && readnone)`
> > 
> Maybe `readonly` is sufficient given that volatile/atomic read accesses are considered writes, right?
That's a good point, in general we need to be more careful.

Though perhaps using `mustprogress` as an initial proxy to enable/disable this altogether would be helpful to limit the fallout at least a little bit during the transition, not here but at the use in `Local.cpp`. instructions there are only considered dead if there are no side-effects at all to start with.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D94106