On 4 October 2016 at 21:05, Emil Velikov <emil.l.veli...@gmail.com> wrote: > Hi Dave, > > On 4 October 2016 at 02:48, Dave Airlie <airl...@gmail.com> wrote: >> From: Bas Nieuwenhuizen <b...@basnieuwenhuizen.nl> >> >> This adds the basic files for the NIR->LLVM translation layer, >> along with some hopefully generic code to load the binary >> result, and other helpers required. >> >> The hope is in the future we could share this with an >> GL_ARB_spirv implementation or a push to replace TGSI >> with NIR in radeonsi. >> > Some of this code is copied from/based on existing one in mesa. Please > mention so in the commit summary and the respective files ?
Okay I've add that in the tops of the files, for some of them the code has changed quite a bit, but at least two are definitely based on other files. >> +AM_CPPFLAGS = \ >> + $(VALGRIND_CFLAGS) \ >> + $(DEFINES) \ >> + -I$(top_srcdir)/include \ >> + -I$(top_builddir)/src \ >> + -I$(top_srcdir)/src \ >> + -I$(top_builddir)/src/compiler \ >> + -I$(top_builddir)/src/compiler/nir \ >> + -I$(top_srcdir)/src/compiler \ >> + -I$(top_srcdir)/src/mapi \ >> + -I$(top_srcdir)/src/mesa \ >> + -I$(top_srcdir)/src/mesa/drivers/dri/common \ >> + -I$(top_srcdir)/src/gallium/auxiliary \ >> + -I$(top_srcdir)/src/gallium/include >> + > I'm leaning that at least some of the above can be nuked, but if it's > too much of a hassle just add a comment on top - XXX/TODO or other. I've just added a TODO, it's all fun and games until you hit main/macros.h. I'm envisaging a future where I'm not including this, anv uses macros from it, but only includes it via the brw_compiler.h file. I think migrating more stuff to util/macros.h might make life better. >> +AM_CFLAGS = -Wno-override-init -msse2 \ > From a curtecy skim though, neither of these two are required. Please drop > them. Done. > >> + $(VISIBILITY_CFLAGS) \ >> + $(PTHREAD_CFLAGS) \ >> + $(LLVM_CFLAGS) \ >> + $(LIBELF_CFLAGS) >> + >> +AM_CXXFLAGS = \ >> + $(VISIBILITY_CXXFLAGS) \ >> + $(MSVC2013_COMPAT_CXXFLAGS) \ > I don't think we're about to build things with MSVC anytime soon, so > we can drop this line. Done. >> +AMD_COMPILER_SOURCES := \ >> + ac_binary.c \ >> + ac_llvm_util.c \ >> + ac_nir_to_llvm.c \ >> + ac_llvm_helper.cpp > Please list all the files. > > ac_binary.c > ac_binary.h > ac_llvm_helper.cpp > ac_llvm_util.c > ac_llvm_util.h > ac_nir_to_llvm.c > ac_nir_to_llvm.h > ac_radeon_winsys.h > I've realised I can kill ac_radeon_winsys.h so I've done this and done that as well. > > >> +#pragma once >> + > Please don't use pragma once, but ifdef FOO/define FOO guards. We have a lot of these in the tree already, I don't envisage building this with any sort of old compilers since it depends on llvm 3.9, which requires a modern compiler to build, but pragma once goes back a long way, and unless someone wants to purge Mesa of all of them I'd prefer to use them. Dave. _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
