[PATCH] LTO: add API to set strategy for -internalize

[Duncan P. N. Exon Smith]

On 2014 Jan 12, at 23:50, Justin Bogner <mail at justinbogner.com> wrote:

> "Duncan P. N. Exon Smith" <dexonsmith at apple.com> writes:
>> Nick: the new patch (attached below) is updated as per our
>> conversation.  Thanks for the review!
>> 
>> Can anyone review the LLVM side?
> 
> LGTM with a couple minor things:

Committed in r199191.  See my comments below.

>> -    explicit InternalizePass();
>> -    explicit InternalizePass(ArrayRef<const char *> ExportList);
>> +    explicit InternalizePass(bool OnlyHidden = false);
>> +    explicit InternalizePass(ArrayRef<const char *> ExportList, bool OnlyHidden);
> 
> Why give one of these a default and not the other?

Since InternalizePass is local to the file, I only implemented the constructors that are actually used.

- createInternalizePass() always specifies OnlyHidden.
- The default constructor is invoked by INITIALIZE_PASS.

I committed without changing this, but let me know if you still think I should add OnlyHidden=false to the ExportList constructor.

>> @@ -99,21 +101,26 @@ void InternalizePass::LoadFile(const char *Filename) {
>>   }
>>   while (In) {
>>     std::string Symbol;
>>     In >> Symbol;
>>     if (!Symbol.empty())
>>       ExternalNames.insert(Symbol);
>>   }
>> }
>> 
>> static bool shouldInternalize(const GlobalValue &GV,
>> -                              const std::set<std::string> &ExternalNames) {
>> +                              const std::set<std::string> &ExternalNames,
>> +                              bool OnlyHidden) {
>> +  // Only internalize hidden symbols.
>> +  if (OnlyHidden && !GV.hasHiddenVisibility())
>> +    return false;
>> +
> 
> The comment is confusing here. It might read better as "Check if we're
> only internalizing hidden symbols", but I'm not convinced it adds
> anything to the code anyway.

Removed the comment.

Thanks for the review!