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

[Jordan Justen]

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?

Thanks,

-Jordan