[llvm-commits] [PATCH] LTO code generator options

[Daniel Dunbar]

On Fri, Nov 20, 2009 at 7:22 AM, Rafael Espindola <espindola at google.com> wrote:
>> StringRef::split would do.
>> The issue here is: SubtargetFeatures class uses std::strings and has a
>> helper method Split which is used in few other places.
>> I'm trying to follow this. I don't like the idea to use StringRef::split
>> only in this particular place.
>> However, I wouldn't mind to re-factor the SubtargetFeatures class to use
>> StringRef instead of std::strings and prepare another patch. Shell I do so?

Please!

> You don't need to. If Split is used elsewhere I agree it is better to
> use it in here too.

Uh, why? We should kill off duplicate code.

 - Daniel

>>> You say features are normalized, why does hasFeature needs to convert
>>> to lowercase and strip flags?
>>
>> Features are normalized inside the SubtargetFeatures class. hasFeature gets
>> a string which also should be normalized to compare with the already
>> normalized features.
>> We need to strip the flag because we do not care was this feature set on or
>> off, we simply want to know if it was set or not.
>> But if you have the question, I'll better add a comment there to clarify
>> this. How about something like this: "Normalize the given string to compare
>> with the normalized features, and strip the flag since we are checking was
>> this feature set at all without checking was it set on or off."?
>
> I think I am still missing something. First on the flag, what is the
> expected result of doing AddFeature("foo", true); AddFeature("foo",
> false). The patch implements a "first one wins". Is that correct?
> Assuming that is the desired behavior I understand the calls to
> StripFlag, but the call to LowercaseString still looks redundant:
>
> *) hasFeature is called only from AddFeature
> *) the string passed to it was generated with
> PrependFlag(LowercaseString(String), IsEnabled)
>
> A comment explaining this should do.
>
>>> The comment about "after all explicit feature settings" is a future
>>> reference, right? I don't see any feature being set :-)
>>
>> You are right. This is a future reference. Next patch will add explicit
>> settings of cpu and attributes. We just need to get there. :)
>>
>>> The method SubtargetFeatures::AddFeatures(const cl::list<std::string>
>>> &List) is not being used, is it?
>>
>> I didn't find a way to add the usage in this patch without adding a lot of
>> other changes to get it work. This will be in the next patch and the usage
>> will look like this in the LTOCodeGenerator.cpp file:
>>
>> ...
>> +static cl::list<std::string> MAttrs("mattr",
>> +  cl::CommaSeparated,
>> +  cl::desc("Target specific attributes (see -mattr=help for details)"),
>> +  cl::value_desc("a1,+a2,-a3,..."));
>> +
>> ...
>>
>> bool LTOCodeGenerator::determineTarget(std::string& errMsg)
>> ...
>> +        // Prepare subtarget feature set for the given command line
>> options.
>> +        SubtargetFeatures features;
>> ...
>> +        if (!MAttrs.empty())
>> +            features.AddFeatures(MAttrs);
>> +
>> +        // Set the rest of features by default.
>> +        // Note: Please keep this after all explict feature settings to
>> make sure
>> +        // defaults will not override explicitly set options.
>> +
>>  features.AddFeatures(SubtargetFeatures::getDefaultSubTargetTripleFeatures(_targetTriple));
>> ...
>>
>> Which also fill that "keep this after all explict feature settings" comment
>> with meaning.
>>
>> Is it Ok to ssubmit this patch?
>
> OK with the comment explaining the addFeature, hasFeature behavior.
>
>> Cheers,
>> Viktor
>
>
> Cheers,
> --
> Rafael Ávila de Espíndola
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>