[PATCH] D26348: Allow convergent attribute for function arguments

[Mehdi AMINI via llvm-commits]

mehdi_amini added inline comments.


================
Comment at: include/llvm/IR/Intrinsics.td:54
+// intrinsic may not be modified in a way that makes the argument value
+// control-dependent on additional values.
+class Convergent<int argNo> : IntrinsicProperty {
----------------
nhaehnle wrote:
> mehdi_amini wrote:
> > nhaehnle wrote:
> > > jlebar wrote:
> > > > This definition actually seems fairly clear to me, if we s/control-//.  Is there a reason we don't want to use this as our canonical definition?
> > > I'd be happy with it. When I posted something like this on the mailing list initially, Mehdi asked for clarification :)
> > My concern on the mailing was the original definition was based on "uniformity".
> > 
> > Anyway, what about this example:
> > 
> > ```
> > if (cond) {
> >   Tmp1 = ...
> >   Foo(Tmp1 [convergent])
> > } else {
> >   Tmp2 = ...
> >   Foo(Tmp2 [convergent])
> > }
> > ```
> > 
> > I think your definition does not prevent:
> > 
> > ```
> > if (cond) {
> >   Tmp1 = ...
> > } else {
> >   Tmp2 = ...
> > }
> > 
> > Tmp3 = phi [Tmp1, Tmp2]
> > 
> > if (cond) {
> >   Foo(Tmp3 [convergent])
> > } else {
> >   Foo(Tmp3 [convergent])
> > }
> > ```
> > 
> > However it breaks uniformity.
> That transform is allowed. It will cause some inefficiencies in the code generation, but the semantics are indeed okay.
> 
> At each call site of Foo, the argument is "dynamically uniform" using the language of the GLSL specification, i.e. all threads that actually go through the call site will see the same value (of course assuming that this was true for the original program). So maybe it's really just a matter of finding the right wording.
OK let's just do one extra transformation:

```
if (cond) {
  Tmp1 = ...
} else {
  Tmp2 = ...
}

Tmp3 = phi [Tmp1, Tmp2]
Foo(Tmp3 [convergent])
```


https://reviews.llvm.org/D26348