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

[Francisco Jerez]

Samuel Iglesias Gonsálvez <siglesias at igalia.com> writes:

> 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?
>> 
>
Hi Samuel,

> 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.
>

That sounds roughly right to me.

> 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.

I think these should be fine because GLSL shaders are not expected to
call the intrinsic explicitly so your lowering pass can just keep a set
of pointers to the intrinsics privately.  AFAIK there would be no use
for adding the ir_function nodes to the symbol table.

> * We are finding several problems implementing this approach that makes
> us think that we are missing something.
>
I don't think you're missing anything, it's just that the GLSL
front-end's infrastructure for dealing intrinsics is close to
nonexistent...

> 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 212 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150703/d3355732/attachment-0001.sig>