[PATCH] D69498: IR: Invert convergent attribute handling

[Johannes Doerfert via Phabricator via cfe-commits]

jdoerfert added a subscriber: hfinkel.
jdoerfert added a comment.

In D69498#1727419 <https://reviews.llvm.org/D69498#1727419>, @mehdi_amini wrote:

> In D69498#1727080 <https://reviews.llvm.org/D69498#1727080>, @jdoerfert wrote:
>
> > Let me quote @arsenm here because this is so important: "Basically no frontend has gotten this right, including clang and non-clang frontends." For me, that statement alone is reason to change the status quo.
>
>
> I am not convinced by this: this seems overly simplistic. The fact that convergent wasn't rolled out properly in frontend can also been as an artifact of the past.


Evidence suggest this problem will resurface over and over again, calling it an artifact of the past is dangerous and not helpful.

> There is a logical gap between this and concluding that the only solution is to change the default.

There is no gap. Making the default restrictive but sound will prevent various kinds of errors we have seen in the past. 
Assuming we could somehow "not repeat the errors in the future" is in my opinion the unrealistic view.

> For instance someone mentioned a pass that could be inserted as the very beginning of the pipeline by any GPU compiler and add the convergent attribute everywhere.

I talked to @hfinkel earlier about this and his idea was to use a pass (or IRBuilder mode, or sth.) to do the opposite: opt-into a "subset" of LLVM-IR behaviors.
The idea is that if you want just a low-level C for CPUs, you run this pass or enable this mode and you get the appropriate LLVM-IR. That would for this patch mean
you get `noconvergent` everywhere but in the future you could get other attributes/changes as well.

>>> As of the frontend, it seems to me that this is just about structuring the code-path for function emission to define the right set of default attribute. It isn't clear to me why a refactoring of the frontend isn't a better course of actions here.
>> 
>> Whatever we do, there will be consequences for current and future front-ends. At the end of the day, the underlying question we need to answer here is: Do we want to have "optimization with opt-in soundness" or "soundness with opt-in optimizations", and I would choose the latter any time.
> 
> This view seems appropriate to me only if you consider a GPU (or SIMT) compiler alone. Another view (which seems to be what @rjmccall has) is that SIMT/GPU is a marginal use-case and "soundness" is already existing for most LLVM use-cases.

I'm strongly opposing the idea to marginalize GPU, or any accelerator, targets. This is not only splitting the community by making it hostile towards some who have to work around whatever is deemed "the main use-case", it is also arguably a position of the past for many of us. Convenience for people that do not care about accelerators is a goal we should have in mind for sure, but it should not oppose a reasonable and sound support of accelerators.


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

https://reviews.llvm.org/D69498