[PATCH] D26348: Allow convergent attribute for function arguments

Matias Goldberg via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Sun Feb 12 12:38:12 PST 2017


dark_sylinc added a comment.

Ok!

So I landed here after a referral. We were literally hit by this bug in LLVM (https://bugs.freedesktop.org/show_bug.cgi?id=99780 <https://bugs.freedesktop.org/show_bug.cgi?id=99780>)

If anyone needs something tangible to know what a miscompiled program looks like due to a missing convergent argument, this is what the bug looks like:
http://imgur.com/43tIuI6 <http://imgur.com/43tIuI6>
http://imgur.com/iBhxoBj <http://imgur.com/iBhxoBj>

>From reading this thread I see there are a few problems with the terms, some confussions, as well as understanding why this is needed.

So I'm going to chime in with an explanation (feel free to add it to the code) in hope of speeding up the acceptance/enhancement of this patch:

GCN executes a 'wavefront'. Each wavefront consists 64 'waves' (aka threads). In NVIDIA's lingo, wavefront = warp and wave = thread; and 1 warp consists of 32 threads (though actualy numbers could vary within architectures and is not relevant).

If a video cards needs to process 128 pixels, it will likely do so using at least 2 wavefronts (2 wavefronts x 64 waves = 128).
OpenGL uses the term "uniform" to refer to parameters that will be constant for //all wavefronts// within the same dispatch.
The problem being addressed here is not uniformity in OpenGL terms, but rather convergence. Convergence is a parameter that must be the same for all waves within the same wavefront, but it is ok if it is different across different wavefronts.

The graph illustrates the idea much better than words:
F3076081: Convergence.png <https://reviews.llvm.org/F3076081>

Terms get messier if we add DirectX terminology, because Microsoft uses the term "const" to refer to what OpenGL understands as "uniform". In fact, DirectX treats uniforms as a synonym of convergence <http://asawicki.info/news_1608_direct3d_12_-_watch_out_for_non-uniform_resource_index.html>.
I'd suggest you stick to OpenGL terms for consistency with your surrounding ecosystem.

DX12's HLSL (via Shader Model 5.1) allows programmers to explicitly tag variables that require convergence as such (by default DX12 assumes variables do not need convergence), as this can lead to performance optimizations if we promise in advance our reads won't diverge.
One such example is rendering sprites via textured quads (1 quad = 2 triangles): Every sprite may have its own texture (and therefore index a different texture). However we promise that each triangle will read the same texture. Due to rasterization ordering rules, different triangles won't be put together in the same wavefront therefore giving us the ability to lift away this restriction and have the compiler produce faster code.

Unfortunately GLSL doesn't have this modifier (yet?), but it also assumes variables do not need convergence, just like DX12. The problem however, is that LLVM is compiling code incorrectly:

As per both OpenGL & DirectX rules, the following does not need to have convergence enforced at compiler level:

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

However the following DOES require to enforce convergence at compiler level:

  in int myIndex;
  sampler2D myTextures[2];
  
  if( myIndex == 0 )
      texture( myTextures[0], uv );
  else if( 
      texture( myTextures[1], uv );

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.

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".
It's like calling a C variable "const" because it can mutate. I think this contradiction is what's causing a buzzing noise in some of the reviewers.

I propose instead:

1. Reserve Keyword "convergent_branch". If a variable is tagged as such, it means additional optimizations can be done (that could be dangerous if the programmer isn't careful). In C terms, tagging a variable "convergent_branch" is analogous to tagging a C pointer with __restrict in terms of programmer responsability.
2. Implement keyword "divergent_branch" (what the current patch refers to as "convergent"). If a variable is tagged divergent_branch, it means the compiler must go to great lengths to ensure correct output even if branches diverges.
3. Reserve the keywords "divergent_index" & "convergent_index" for future use.
4. Tagging a variable as both convergent_branch and divergent_branch at the same time would be illegal and contradictory.
5. Tagging a variable as both convergent_index and divergent_index at the same time would be illegal and contradictory.
6. It is legal for a variable to be both divergent_index and convergent_branch at the same time, or both convergent_index and divergent_branch.
7. Full level paranoia would be to tag a variable as both divergent_index & divergent_branch
8. Maximum optimization can be obtained by tagging a variable as both convergent_index & convergent_branch

**About terms:**
**What does a variable that is marked as divergent mean?**
It means the compiler must enforce results are correct even if the execution diverges within the same wavefront. We say it is divergent because the compiler must enforce convergence at code level.
**What does a variable that is marked as convergent mean?**
It means the compiler is free to optimize code in a way that could result in improper output if the programmer doesn't guarantee convergence via external means (e.g. by the data it will feed to the program, by exploiting rasterization rules, etc)

I hope this explanation and proposal (to change the name of the keyword currently being used) shed lights to all of you; and accelerates the inclusion of the final patch.
I'm not a language lawyer, I'm not an LLVM expert either. I'm just a graphics guy who was pissed enough to care.

Cheers
Matias


https://reviews.llvm.org/D26348





More information about the llvm-commits mailing list