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... > 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
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev