[PATCH] D26348: Allow convergent attribute for function arguments

[Mehdi AMINI via Phabricator via llvm-commits]

mehdi_amini added a comment.

In https://reviews.llvm.org/D26348#674624, @dark_sylinc wrote:

> Currently, the problem is that right now LLVM in the latter case is "optimizing" the code to execute like the former code to save one image_sample instruction, which is wrong.


I'd like to clarify here that the existing bug is not really in the LLVM optimizer, but rather in the fact that graphic frontend are not emitting these calls with the right set of conservative attributes. The immediate fix would be for them to change this and not leave the optimizer mess up with these operations. (And not to put pressure on LLVM to adopt this patch as a correctness fix, it is not, or only incidentally).
(Of course being conservative means losing some optimizations possibilities in some cases, and the purpose of this patch is to help getting these back with finer modeling of the constraint).

> Right now what I see wrong about the patch' terminology is that it marks variables requiring to enforce convergence at compiler level as "convergent". That's exactly the opposite! It should be tagged as "divergent". If a variable is divergent, then the compiler must enforce convergence. If a variable is already convergent, then it can perform aggressive optimizations. You don't enforce convergence on something that is already convergent!

I'm not sure I follow. We're maybe not talking about the same thing. We not constraining variables here, but instead "operations" if I get it correctly (going back to this every two months isn't making it easy). 
IIUC, taking your examples:

  int myIndex;
   sampler2D myTextures[2];
   texture( myTextures[myIndex], uv ); //If myIndex diverges, it's programmer's fault.

The information we're trying to carry right now with this patch,  is not about `myTextures[myIndex]`, but about the fact that a call to `texture` needs to preserve the convergence on its first argument.

We are *not* carrying any information supplied by the programmer about the convergence property of a variable. We can just assume that in the source program the first argument of a call `texture` is convergent, and we need to maintain it.

> I propose instead:
>  [snip]

None of it seems to fit LLVM IR but looks rather like a language specification to me. 
As a starting point, LLVM IR doesn't have any concept of "variables".  We have SSA values and memory (which can be stack allocations or not).

It also seems to me that you're trying to bring the source-level programmer annotations of convergence on source program variables, in order to enable more optimizations. There may be some optimizations to do around this, but that seems orthogonal to what this patch is doing which is, as far as I understand, about modeling the correctness constraint associated with a call to `texture` (i.e. to express what is the property that needs to be maintained on the first argument).


https://reviews.llvm.org/D26348