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

[Viktor Kutuzov]

Hello Rafael,

> Can you give the StringRef::split method a try?

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

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

Cheers,
Viktor

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


>> This patch adds two SubtargetFeatures::AddFeatures methods which accept a
>> comma-separated string or already parsed command line parameters as input,
>> SubtargetFeatures::hasFeature method which cheks if given feature is already
>> set, and some code re-factoring to use these new methods.
>
> Can you give the StringRef::split method a try?
>
> You say features are normalized, why does hasFeature needs to convert
> to lowercase and strip flags?
>
> The comment about "after all explicit feature settings" is a future
> reference, right? I don't see any feature being set :-)
>
> The method SubtargetFeatures::AddFeatures(const cl::list<std::string>
> &List) is not being used, is it?
>
>> Best regards,
>> Viktor
>
> Cheers,
> -- 
> Rafael Ávila de Espíndola
>