[PATCH] D26348: Allow convergent attribute for function arguments

Hal Finkel via llvm-commits llvm-commits at lists.llvm.org
Thu Nov 10 09:39:51 PST 2016


hfinkel 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 {
----------------
mehdi_amini wrote:
> nhaehnle wrote:
> > hfinkel wrote:
> > > mehdi_amini wrote:
> > > > nhaehnle wrote:
> > > > > mehdi_amini wrote:
> > > > > > 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])
> > > > > > ```
> > > > > That transformation is incorrect, and this RFC patch already disables it (in SimplifyCFG/tail sinking). A simpler example would be just:
> > > > > 
> > > > >   if (cond) {
> > > > >     Tmp1 = Foo(v [convergent])
> > > > >   } else {
> > > > >     Tmp2 = Foo(v [convergent])
> > > > >   }
> > > > >   Tmp3 = phi [Tmp1, Tmp2]
> > > > > 
> > > > > to
> > > > > 
> > > > >   Tmp3 = Foo(v [convergent])
> > > > > 
> > > > > This transform is incorrect in general because cond could be something like `v == 0`, where v is an int that can only take values 0 or 1.
> > > > > 
> > > > > Formally, the constraint formulation with runs r1 and r2 forbids this transform because you can choose r1 = { cond = true, v = A }, r2 = { cond = false, v = B }. These runs are compatible in the original program, but incompatible in the transformed program. (This is basically what happens in the linked bug report.)
> > > > > 
> > > > > The transform would be correct if the branch condition were proved to be "uncorrelated" with the convergent argument. This can sometimes be done with target-specific knowledge. This target-specific knowledge effectively restricts which pairs of runs need to be considered in the runs-based constraint formulation.
> > > > I'm addressing the definition proposed here: " The values to this argument are convergent, and calls to the intrinsic may not be modified in a way that makes the argument value control-dependent on additional values."
> > > > 
> > > > I don't see how any of *this* two definition forbids the transformation I mentioned.
> > > > 
> > > > Or the one in LangRef: "When a function argument has the ``convergent`` attribute, a call to the function may only be transformed in a way that does not add additional sources of divergence to the argument."
> > > > 
> > > > It is using the concept of "divergence" without defining it, so this one is more iffy, but still not clear.
> > > > It is using the concept of "divergence" without defining it, so this one is more iffy, but still not clear.
> > > 
> > > Yea,  we normally talk about this in terms of adding control dependencies, but we'll need to carefully define what we mean here.
> > Okay, so how about in this location in Intrinsics.td we remove the "control" from "control-dependent" and refer explicitly to the IR attribute? Then at least it's clear that the proper definition is located in one specific place. Maybe as short as:
> > 
> >   // Convergent - The values to this argument are convergent. This corresponds
> >   // to the IR function argument attribute of the same name.
> > 
> > Then we can focus on the language in LangRef - any feedback on that?
> The quote I posted just above about using "source of divergence" comes from LangRef. 
No, because there are important differences. Both refer to control dependencies, but on a function it refers to control dependencies on the execution of the function, but on the value, it is talking about control dependencies on the value being passed into the function, right?



https://reviews.llvm.org/D26348





More information about the llvm-commits mailing list