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

[Iago Toral]

On Mon, 2015-06-29 at 00:11 -0700, 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?

Sure, that sounds reasonable.

Iago