[Mesa-dev] [PATCH 07/11] ac/radv: move llvm compiler info to struct and init in one place

[Dave Airlie]

On 30 June 2018 at 13:30, Marek Olšák <maraeo at gmail.com> wrote:
> On Tue, Jun 26, 2018 at 11:58 PM, Dave Airlie <airlied at gmail.com> wrote:
>> From: Dave Airlie <airlied at redhat.com>
>>
>> This creates a common per-thread compiler info struct, and adds
>> the init code to it. This is mostly ported from radeonsi.
>>
>> The common info struct is used in radv first and replaces the
>> current code.
>> ---
>>  src/amd/common/ac_llvm_util.c     | 50 +++++++++++++++++++++++++++++++
>>  src/amd/common/ac_llvm_util.h     | 14 +++++++++
>>  src/amd/vulkan/radv_nir_to_llvm.c | 39 ++++++++++--------------
>>  src/amd/vulkan/radv_private.h     |  7 ++---
>>  src/amd/vulkan/radv_shader.c      | 16 +++++-----
>>  5 files changed, 91 insertions(+), 35 deletions(-)
>>
>> diff --git a/src/amd/common/ac_llvm_util.c b/src/amd/common/ac_llvm_util.c
>> index dd2469d4606..85dc9d72a5c 100644
>> --- a/src/amd/common/ac_llvm_util.c
>> +++ b/src/amd/common/ac_llvm_util.c
>> @@ -188,6 +188,56 @@ LLVMPassManagerRef ac_init_passmgr(LLVMTargetLibraryInfoRef target_library_info,
>>         return passmgr;
>>  }
>>
>> +bool ac_llvm_compiler_init(struct ac_llvm_compiler_info *info,
>> +                          bool add_target_library_info,
>> +                          enum radeon_family family,
>> +                          enum ac_target_machine_options tm_options)
>> +{
>> +       memset(info, 0, sizeof(*info));
>> +       info->tm = ac_create_target_machine(family, tm_options, &info->triple);
>> +       if (!info->tm)
>> +               return false;
>> +
>> +       /* Get the data layout. */
>> +       LLVMTargetDataRef data_layout = LLVMCreateTargetDataLayout(info->tm);
>> +       if (!data_layout)
>> +               goto fail;
>> +       info->data_layout = LLVMCopyStringRepOfTargetData(data_layout);
>> +       LLVMDisposeTargetData(data_layout);
>> +
>> +#if HAVE_LLVM < 0x0700
>
> This #if is not needed. You already have a bool flag coming from radv.
> You can modify the bool value in radv.
>
>> +       if (add_target_library_info)
>> +#endif

The if flag is to encapsulate things in this file, if llvm target
library info is broken,
and we have to leak something, then the driver indicates if can deal with the
leak or not with the flag. If the leak isn't going to happen then
we've encapsulate
the leak problem in one place.

Having the HAVE_LLVM in the driver to denote that LLVM is broken seems wrong,
granted not having broken llvm in the first place would have been nice.

I'll think about if I can make it clearer at least why this is here.

Dave.