[Mesa-dev] [PATCH] clover: Introduce CLOVER_COMPILER_OPTIONS

[Vedran Miletić]

On 08/30/2016 09:27 AM, Serge Martin wrote:
> On Tuesday 30 August 2016 01:20:55 Vedran Miletić wrote:
>> Options specified via the CLOVER_COMPILER_OPTIONS shell variable are
>> appended to the compiler options specified by the OpenCL program (if
>> any).
>>
>> Signed-off-by: Vedran Miletić <vedran at miletic.net>
>> ---
>>  docs/envvars.html                                     | 2 ++
>>  src/gallium/state_trackers/clover/llvm/invocation.cpp | 9 +++++++--
>>  2 files changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/docs/envvars.html b/docs/envvars.html
>> index 6d79398..52835b6 100644
>> --- a/docs/envvars.html
>> +++ b/docs/envvars.html
>> @@ -224,6 +224,8 @@ Mesa EGL supports different sets of environment
>> variables.  See the <li>GALLIUM_DUMP_CPU - if non-zero, print information
>> about the CPU on start-up <li>TGSI_PRINT_SANITY - if set, do extra sanity
>> checking on TGSI shaders and print any errors to stderr.
>> +<li>CLOVER_COMPILER_OPTIONS - allows specifying additional compiler
> 
> I think CLOVER_EXTRA_COMPILER_OPTIONS would be better
> 

Agree.

>> options. +    Specified options are appended after the options set by the
>> OpenCL program. <LI>DRAW_FSE - ???
>>  <LI>DRAW_NO_FSE - ???
>>  <li>DRAW_USE_LLVM - if set to zero, the draw module will not use LLVM to
>> execute diff --git a/src/gallium/state_trackers/clover/llvm/invocation.cpp
>> b/src/gallium/state_trackers/clover/llvm/invocation.cpp index
>> 5490d72..748850f 100644
>> --- a/src/gallium/state_trackers/clover/llvm/invocation.cpp
>> +++ b/src/gallium/state_trackers/clover/llvm/invocation.cpp
>> @@ -196,11 +196,16 @@ clover::llvm::compile_program(const std::string
>> &source, const std::string &target,
>>                                const std::string &opts,
>>                                std::string &r_log) {
>> +   const char *extra_opts_env = getenv("CLOVER_COMPILER_OPTIONS");
> 
> please use debug_get_option
> 

OK. I will also provide a patch for the rest of Mesa with getenv(),
os_get_option() -> debug_get_option() and mention that preference in the
docs.

>> +   std::string extra_opts;
>> +   if (extra_opts_env)
>> +       extra_opts = std::string(extra_opts_env);
>> +
>>     if (has_flag(debug::clc))
>> -      debug::log(".cl", "// Options: " + opts + '\n' + source);
>> +      debug::log(".cl", "// Compiler options: " + opts + " " + extra_opts +
>> '\n' + source);
> 
> Please don't change the "// Options:" string
> 

Not feeling strongly about this. OK.

>>
>>     auto ctx = create_context(r_log);
>> -   auto c = create_compiler_instance(target, tokenize(opts + " input.cl"),
>> +   auto c = create_compiler_instance(target, tokenize(opts + " " +
>> extra_opts + " input.cl"), r_log);
>>     auto mod = compile(*ctx, *c, "input.cl", source, headers, target, opts,
>>                        r_log);
> 
> You forgot to do the same in the linker part.
> 
> I think it will be less change if you use a variable with all the options.
> 
> What about renaming the opts arg to user_opts and then grab the var this way :
> 
> const std::string opts = user_opts + " " + 
> debug_get_option("CLOVER_EXTRA_COMPILER_OPTIONS", "");
> 

Could indeed work, good idea. Patch v2 incoming.

Thanks for the review.

Vedran

> 
> 
> 
> 
> 
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 

-- 
Vedran Miletić
vedran.miletic.net