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

[Viktor Kutuzov]

Hello Rafael,

I have removed the hasFeature method for now since it triggers those kind of questions.
Later I may add it along with the clear use case, but for now the result of doing AddFeature("foo", true); AddFeature("foo", false)
remains unchanged and is up to the TargetMachine implementation (the last one wins).

Thanks a lot for reviewing the patch.
It is commited as http://llvm.org/viewvc/llvm-project?rev=89516&view=rev

Now everything is reaady for the target triple overriding.

Please find the patch attached.
This patch allows setting target triple (-mtriple) as a command line argument as well as -mcpu, and -mattr.

Best regards,
Viktor

----- Original Message ----- 
From: "Rafael Espindola" <espindola at google.com>
To: "Viktor Kutuzov" <vkutuzov at accesssoftek.com>
Cc: "Commit Messages and Patches for LLVM" <llvm-commits at cs.uiuc.edu>
Sent: Friday, November 20, 2009 7:22 AM
Subject: Re: [llvm-commits] [PATCH] LTO code generator options


>> 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
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: llvm-lto-codegen-target_triple_override.diff
Type: application/octet-stream
Size: 9382 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20091123/3cf04ae3/attachment.obj>