[PATCH] D38113: OpenCL: Assume functions are convergent

[Anastasia Stulova via Phabricator via cfe-commits]

Anastasia added a comment.

In https://reviews.llvm.org/D38113#878840, @jlebar wrote:

> > Yes, that's why if it would be responsibility of the kernel developer to specify this explicitly we could avoid this complications in the compiler. But if we add it into the language now we still need to support the correctness for the code written with the earlier standards. And also it adds the complexity to the programmer to make sure it's specified correctly. But I think it is still worth discussing with the spec committee.
>
> To me this seems like a small complication in the compiler to avoid an extremely easy bug for users to write.  But, not my language.  :)


Yes, I think I would perhaps argue for inclusion of non-convergent instead since this option seems to make more sense.

> 
> 
>> The deduction of convergent is indeed tricky. So if there is any function in the CFG path which is marked as convergent ( or "non-convergent") this will have to be back propagated to the callers unless we force to explicitly specify it but it would be too error prone for the kernel writers I guess.
> 
> This probably isn't the right forum to discuss proposals to change the LLVM IR spec.  But if you want to propose something like this, please cc me on the thread, I probably have opinions.  :)

Will do! If we have bigger use case it would be easier to get accepted. I will check with the OpenCL community first and see if there is an agreement internally.

> 
> 
>> Btw, what is the advantage of having "non-convergent" instead and why is the deduction of convergent property more complicated with it?
> 
> The advantage of switching LLVM IR to non-convergent would be that front-ends wouldn't have the bug that arsenm is fixing here.  "Unadorned" IR would be correct.  And, in the absence of external or unanalyzable indirect calls, you'd get the same performance as we get today even if you had no annotations.

Yes, I see this sounds more reasonable indeed. Btw, currently LLVM can remove `convergent` in some cases to recover the performance loss?

> The complexity I was referring to occurs if you add the non-convergent attribute and keep the convergent attr.  I don't think we want that.

I think keeping both would add more confusions and hence result in even more errors/complications.

> But I'm not really proposing a change to the convergent attribute in LLVM IR -- it's probably better to leave it as-is, since we all understand how it works, it ain't broke.

But at the same time since we already know what we needed redesign should be easier. Alternative option would be to add convergent during IR generation as default option and no attribute where `non-convergent` is set. This way at least we give programmer a way to achieve higher performance. But of course it wouldn't be ideal to be inconsistent with the IR. Currently as I can see there is a little use of convergent in the frontend since we set it for all functions anyways. The problem is that it wouldn't be possible to remove it immediately. But we can at least deprecate it for a start.


https://reviews.llvm.org/D38113