[PATCH] D69498: IR: Invert convergent attribute handling

Wed Oct 30 00:03:03 PDT 2019

mehdi_amini added a comment.

In D69498#1725867 <https://reviews.llvm.org/D69498#1725867>, @jdoerfert wrote:

> In D69498#1725819 <https://reviews.llvm.org/D69498#1725819>, @mehdi_amini wrote:
>
> > Maybe we can start by looking into the motivation for this patch:
> >
> > > There is a burden on frontends in environments that care about convergent operations to add the attribute just in case it is needed. This has proven to be somewhat problematic, and different frontend projects have consistently run into problems related to this.
> >
> > Can you clarify what kind of problems? Do you have concrete examples?
>
>
> The frontend has to put convergent on everything that is not known to be non-convergent right now. That is, the front-end and all function generating constructs need to know about convergent to preserve correctness.

Right, but that does not seem like a problem to me so far: a GPU frontend must add a gpu-specific attribute on every function seems like a reasonable constraint to me.

> Frontends in the past put convergent only on barriers or similar calls but not on all functions that might transitively call barriers, leading to subtle bugs.

Right, but it seems like trying to fix bugs in the frontend at the LLVM does not seems necessarily right: it looks like a big hammer.

In D69498#1725877 <https://reviews.llvm.org/D69498#1725877>, @arsenm wrote:

> Basically no frontend has gotten this right, including clang and non-clang frontends. There's a gradual discovery process that it's necessary in the first place, and then a long tail of contexts where it's discovered it's missing. As I mentioned before, SYCL and OpenMP are both still broken in this regard. For example in clang, convergent wasn't initially added to all functions, and then it was discovered that transforms on indirect callers violated it.

The rule seems fairly straightforward for the frontend: the attributes must be always added everywhere if you're using SIMT.
Originally it likely wasn't thought through for convergent, but at this point this seems behind us.

> Then later it was discovered this was broken for inline asm.

Can you clarify what you mean here and how this change would fix it?

> Then some specific intrinsic call sites were a problem.

Same, I'm not sure I understand this one.

> Then there are special cases that don't necessarily go through the standard user function codegen paths which don't get the default attributes.

> Some odd places where IR is compiled into the user library that didn't have convergent added.

Bug in the user library? Again I'm not sure why it justifies a change in LLVM.

> The short version is the fact that most developers aren't aware of and don't understand the subtleties of convergence, and making sure the front-end does something in all contexts requires a lot of diligence. It's very easy to introduce these difficult to debug bugs when calls are broken by default.

The fact that most developers aren't aware of convergence means a lot of potential bugs in LLVM because passes and transformations won't honor the convergent semantics, but the default for the attribute won't change that.
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.

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

https://reviews.llvm.org/D69498