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

[Iago Toral]

On Fri, 2015-07-03 at 13:23 +0300, Francisco Jerez wrote:
> 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...

Thanks for the input Curro. For what is worth, we identified and fixed
the issues that Samuel was mentioning and we expect to have the new
implementation working soon.

Iago

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