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

[Rafael Espindola]

> 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?

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

>> 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