On Fri, 2015-07-03 at 13:23 +0300, Francisco Jerez wrote: > Samuel Iglesias Gonsálvez <sigles...@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 _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
