On 30/04/18 11:53, Timothy Arceri wrote: > <snip> > I'll reply to the above snipped section in a separate email. > >>> >>> Ignoring the above issue I'm fairly certain adding a dependency on >>> core mesa GL to NIR is a no no so either way these files don't >>> belong in src/compiler/nir >> >> But we already have that dependency, right? I mean gl_program have a >> reference to nir_shader for some time now. > > This is largely historical when NIR was created we had no Vulkan > drivers and it originally lived in src/glsl/nir I have a feeling we > can get rid of most of these old dependencies by moving some GL only > helpers out of the src/compiler/nir dir, I might give this a go.
Ok, thanks. > >> >> And in any case, the implementation of ARB_gl_spirv is based on NIR, >> as right now we don't have an alternative to process SPIR-V. So for >> example, the following patch (already merged on master) [3]: >> mesa/glspirv: Add a _mesa_spirv_to_nir() function >> This is basically a wrapper around spirv_to_nir() that includes >> arguments setup and post-conversion validation. >> >> Is at main/glspirv.c, and calls directly spirv_to_nir. >> >> Although it is true that code would not be called unless the >> extension is enabled, it depends on NIR and it is included on core >> mesa (main/gl_spirv.c). >> >> Well, unless I'm misunderstanding what do you mean for core mesa. I'm >> including mesa/main as core mesa too. > > A dependency on NIR from Mesa is fine, I'm saying NIR (the core > library) shouldn't have dependencies on core Mesa. Ok, thanks for the explanation. > >> >> In any case, I'm not against moving those files to a different >> directory if needed, just trying to understand why doesn't belong there. > > In patch 12 & 16 you are creating a "util" so you can call things from > the helpers you are placing in the nir dir but these are GL only > concepts. The util is not a real util that is shared by the vulkan and > gl compiler but rather you are working around a dependency problem you > have created by locating the uniform linker and other helpers inside > the nir directory. You are simply bundling code into builds of Vulkan > drivers that will never be used. Good point, with this directory structure, Vulkan drivers are getting code that will not use at all. > > If you simply put these files inside src/compiler/glsl/ then you don't > need this util at all. True, but (and sorry for being nitpicky), it sounds strange to me to put code on a glsl directory that are not really related to glsl. > It would probably make more sense now that this extension has come > along if the dir was actually src/compiler/gl/ but its not worth the > hassle of changing that. Ok, I see your point. Yes, changing the name would be a hassle, and adding a new directory (so having both src/compiler/gl and src/compiler/glsl) confusing. So unless someone complains or have a better idea, we would go for your suggestion. But if you don't mind I would wait to do the change until we get review on the code itself, and not only on the placement. > > <snip> _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
