[llvm-commits] FW: [PATCH] Fix to pass options from Gold plugin to LTO codegen

[Nick Lewycky]

Viktor Kutuzov wrote:
> Please find attached the updated patch.
> Is it Ok to submit?

Looks great, please commit!

Nick

> Viktor
> 
> ----- Original Message ----- From: "Nick Lewycky" <nicholas at mxc.ca>
> To: "Viktor Kutuzov" <vkutuzov at accesssoftek.com>
> Cc: "Commit Messages and Patches for LLVM" <llvm-commits at cs.uiuc.edu>
> Sent: Monday, October 26, 2009 9:54 PM
> Subject: Re: [llvm-commits] FW: [PATCH] Fix to pass options from Gold 
> plugin to LTO codegen
> 
> 
> Viktor Kutuzov wrote:
>> Hello Nick,
>> Thanks for the review.
>>
>> Please find the updated patch attached.
>>
>> I didn't remove the lto_codegen_debug_options yet, just marked it as 
>> deprecated.
>> It is actually used in Ada, in files
>>
>> bindings/ada/llvm/llvm_linktimeoptimizer_wrap.cxx
>> bindings/ada/llvm/llvm_link_time_optimizer-binding.ads
> 
> Bah, then it's too late. We released 2.6 with this API which means that 
> it's fixed in stone forever. All of the C API is like that, we will 
> never break backwards-compatibility on the C API.
> 
> Well, given that, why not just call lto_codegen_debug_options? The 
> comment makes it clear that it's "used to pass extra options to the code 
> generator" and that's just what we're doing.
> 
>> Edward, is this fine to use lto_codegen_set_options there instead?
>>
>>> And that would be even better if it took an argc+argv pair
>>
>> Gold plug-in gets all arguments from gold one by one.
>> It is not a big dial to construct argc+argv pair there and pass it 
>> down to the LTO, but LTO internally keeps a vector of char*, so, 
>> argc+argv doesn't make much sense unless we want to replace that 
>> vector with argc+argv pair as well and to change setCodeGenOptions 
>> behavior to set options instead of add them.
> 
> Sure, given that lto_codegen_debug_options takes one option at a time 
> and we're wedded to this API then we may as well use it.
> 
> Please send another patch without the new "lto_codegen_set_options" 
> function by calling the "lto_codegen_debug_options" function instead. 
> The changes to gold-plugin.cpp look great to me!
> 
> Nick
> 
>> If this is fine with everyone, I can prepare this patch, but would 
>> prefer to make it separately from this one since it would change the 
>> API and will break a backward compatibility.
>> What do you think?
>>
>> Thanks,
>> Viktor
>>
>> ----- Original Message ----- From: "Nick Lewycky" <nicholas at mxc.ca>
>> To: "Viktor Kutuzov" <vkutuzov at accesssoftek.com>
>> Cc: "Commit Messages and Patches for LLVM" <llvm-commits at cs.uiuc.edu>
>> Sent: Friday, October 23, 2009 5:56 PM
>> Subject: Re: [llvm-commits] FW: [PATCH] Fix to pass options from Gold 
>> plugin to LTO codegen
>>
>>
>> Viktor Kutuzov wrote:
>>> Hello everyone,
>>>
>>> Please find the patch attached.
>>> This should fix the issue when gold plugin doesn't pass the  
>>> -plugin-opt options down to the LTO codegen.
>>> See the thread "[LLVMdev] getting gold plugin to work?" for more 
>>> details.
>>>
>>> This is the second try.
>>> It looks like the first one was bounced, not sure why.
>>> Sorry if anyone will get it twise.
>>
>> Hi Viktor, this looks like the same patch I already reviewed here:
>> http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20091019/089467.html 
>>
>>
>> To reiterate:
>> "What's the difference between setCodeGenDebugOptions and
>> setCodeGenOption? It looks like they should be merged into one then
>> exposed by lto_codegen_set_option. And that would be even better if it
>> took an argc+argv pair, even if that's a little harder to construct from
>> the gold plugin, many option parsing packages work by removing the
>> options they recognize from argc+argv and leaving the rest in place."
>>
>> Nick
>>
>>
>> ------------------------------------------------------------------------
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>