[Mesa-dev] [PATCH v2 23/82] glsl: Do not do CSE for expressions involving SSBO loads

[Samuel Iglesias Gonsálvez]

On 29/06/15 09:11, Jordan Justen wrote:
> On 2015-06-24 07:36:24, Iago Toral wrote:
>> On Wed, 2015-06-24 at 15:43 +0300, Francisco Jerez wrote:
>>> AFAICT the reason why this (and many of the other changes in GLSL
>>> optimization passes) is needed is because SSBO loads have been
>>> implemented as ir_expression nodes instead of being lowered into
>>> intrinsics (as other side-effectful operations do like
>>> ARB_shader_image_load_store and ARB_shader_atomic_counters).  This
>>> surely broke the assumption of a number of optimization passes that
>>> ir_expression nodes behave as pure functions.  I guess the reason why
>>> you've done it this way is because UBO loads were already being
>>> represented as expressions, so I see why you may have wanted to use the
>>> same approach for SSBOs even though there is a fundamental difference
>>> between the two: UBO loads have no side effects and are constant for a
>>> given set of arguments and a given shader execution, SSBO loads and
>>> stores are not.  SSBO stores couldn't be accommodated into the same
>>> framework so easily, and you decided to create a separate ir node for
>>> them, what seems inconsistent with loads.  Intrinsics would probably
>>> have been a good fit for both loads and stores, and would have made all
>>> these optimization changes unnecessary...
>>>
>>> P.S.: Sorry for the late reply, I was on vacation when I was CC'ed.
>>
>> Right, your assessment about the reasons behind the current
>> implementation is correct. I did not realize of these issues when I
>> decided to go with the current implementation, now it does look like
>> going with GLSL intrinsics would have made things a bit easier. I
>> suppose it would make sense to revisit the implementation in the near
>> future taking your work on arb_shader_image_load_store as a reference.
> 
> While we're waiting for curro's work to land, I was hoping to review
> and let you guys land the first ~30 front end patches. These patches
> would allow some compiler tests to pass if the extension is
> overridden. (Plus, it would take a big chunk out of this large
> series.)
> 
> Unfortunately, I think you should rework the load/store ops as
> intrinsics as recommended by curro.
> 
> Once you have the extension working again with intrinsics, could you
> re-post the early patches before the 'i965' patches start?
> 
> Does this seem like a reasonable plan?
> 

Iago and I are working on defining SSBO load/store as GLSL IR
intrinsincs. After looking at what Francisco did for
ARB_shader_image_load_store, we found some differences.

ARB_shader_image_load_store defines imageStore() and imageLoad() as
built-in functions and they are called explicitly by GLSL shaders. In
our case SSBO load/store are implicit and, because of that, we need to
do a lowering pass when we have all the needed SSBOs information, i.e.
at link time, similar to what we did in the patch series.

Our idea is to make that lowering pass to inject ir_call nodes replacing
the creation of ir_ssbo_store nodes and ssbo_load expressions we had before.

In order to inject those ir_call nodes, we are thinking about doing some
steps similar to how built-in functions are defined but inside the
lowering pass:

1) Create ir_function_signature for the corresponding intrinsic (SSBO
store or load)
2) Create an ir_function with the desired name and add the signature
created in first step to it.
3) Create an ir_call node passing as argument the created
ir_function_signature and the list of variables that SSBO store/load
need to work.
4) Add the new ir_call to the list of IR instructions.

However we don't know if this is the proper approach for several reasons:

* As we are executing the lowering pass in link time, we don't have the
table of symbols (it was deleted before), so we cannot add the created
ir_function to it (like built-in function's definition code does).
* Creating ir_function_signature in the lowering pass doesn't seem right
to us but, as the table of symbols has been deleted, we cannot get it
from other place if it was created before.
* We are finding several problems implementing this approach that makes
us think that we are missing something.

As we don't see any similar implementation in the source code (i.e. no
emission of implicit intrinsics), we don't know if this is the correct
approach to follow or if there is a better way of doing it.

Are we missing something? What do you think?

Sam