[Mesa-dev] [PATCH 1/4] amd/common: add nir->llvm translation.

[Dave Airlie]

On 4 October 2016 at 21:05, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> Hi Dave,
>
> On 4 October 2016 at 02:48, Dave Airlie <airlied at gmail.com> wrote:
>> From: Bas Nieuwenhuizen <bas at 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.