[PATCH] D69498: IR: Invert convergent attribute handling

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

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

> In D69498#1727546 <https://reviews.llvm.org/D69498#1727546>, @jdoerfert wrote:
>
> > 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.
>
>
> Can you provide the "evidence"? So far the only thing I have seen is historical and closely tied to how convergence was rolled out AFAICT.


Recently, as mentioned a few times before, OpenMP target offloading gets/got it wrong long after convergence was rolled out.
r238264 introduced convergent in 2015, r315094 fixed it in part for OpenCL in 2017 (with the idea to remove noduplicate which was also broken!) a year after OpenCL support was started.

>>> There is a logical gap between this and concluding that the only solution is to change the default.
>> 
>> There is no gap.
> 
> Bluntly contradicting what I wrote without apparently trying to understand my point or trying to explain your point with respect to what I'm trying to explain is not gonna get me to agree with you magically, I'm not sure what you expect here other than shutting down the discussion and leaving us in a frustrating disagreement and lack of understanding with this?

You removed my explanation that directly follows my statement. Please do not use my quotes out of context like this to blame me.

To reiterate what actually followed my above quote <https://reviews.llvm.org/D69498/new/#1727546>: If we have a "by default unsound behavior" we will by design have cases we miss, e.g., anything new not added with this special case in mind. So changing the default to the "by construction sound" case, is in my opinion the only way to prevent that. That being said, I already agreed that this might not be necessary for every target, e.g., based on the data layout. However, I do try to bring forward reasons why a "split in semantics" on this level will hurt us as a community and maintainers of the IR continuously.


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

https://reviews.llvm.org/D69498