2011/9/22 Justin Holewinski <[email protected]> > On Thu, Sep 15, 2011 at 6:08 PM, Justin Holewinski < > [email protected]> wrote: > >> On Thu, Sep 15, 2011 at 1:45 PM, Matthieu Monrocq < >> [email protected]> wrote: >> >>> >>> Date: Thu, 15 Sep 2011 15:34:14 +0100 >>>> From: Tobias Grosser <[email protected]> >>>> Subject: Re: [cfe-commits] r139789 - /cfe/trunk/lib/Basic/Targets.cpp >>>> To: Justin Holewinski <[email protected]> >>>> Cc: [email protected] >>>> Message-ID: <[email protected]> >>>> Content-Type: text/plain; charset=ISO-8859-1; format=flowed >>>> >>>> >>>> On 09/15/2011 01:13 PM, Justin Holewinski wrote: >>>> > Author: jholewinski >>>> > Date: Thu Sep 15 07:13:38 2011 >>>> > New Revision: 139789 >>>> > >>>> > URL: http://llvm.org/viewvc/llvm-project?rev=139789&view=rev >>>> > Log: >>>> > PTX: Define target options >>>> Hey Justin, >>>> >>>> this is nice, but you might shorten the code slightly. >>>> >>>> > Modified: >>>> > cfe/trunk/lib/Basic/Targets.cpp >>>> > >>>> > Modified: cfe/trunk/lib/Basic/Targets.cpp >>>> > URL: >>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Basic/Targets.cpp?rev=139789&r1=139788&r2=139789&view=diff >>>> > >>>> ============================================================================== >>>> > --- cfe/trunk/lib/Basic/Targets.cpp (original) >>>> > +++ cfe/trunk/lib/Basic/Targets.cpp Thu Sep 15 07:13:38 2011 >>>> > @@ -916,6 +916,10 @@ >>>> > // FIXME: implement >>>> > return "typedef char* __builtin_va_list;"; >>>> > } >>>> > + >>>> > + virtual bool setFeatureEnabled(llvm::StringMap<bool> &Features, >>>> > + const std::string&Name, >>>> > + bool Enabled) const; >>>> > }; >>>> > >>>> > const Builtin::Info PTXTargetInfo::BuiltinInfo[] = { >>>> > @@ -935,6 +939,91 @@ >>>> > NumNames = llvm::array_lengthof(GCCRegNames); >>>> > } >>>> > >>>> > + bool PTXTargetInfo::setFeatureEnabled(llvm::StringMap<bool> >>>> &Features, >>>> > + const std::string&Name, >>>> > + bool Enabled) const { >>>> > + if (Enabled) { >>>> > + if (Name == "double") >>>> > + Features["double"] = true; >>>> > + else if (Name == "no-fma") >>>> > + Features["no-fma"] = true; >>>> > + else if (Name == "compute10") >>>> > + Features["compute10"] = true; >>>> > + else if (Name == "compute11") >>>> > + Features["compute11"] = true; >>>> > + else if (Name == "compute12") >>>> > + Features["compute12"] = true; >>>> > + else if (Name == "compute13") >>>> > + Features["compute13"] = true; >>>> > + else if (Name == "compute20") >>>> > + Features["compute20"] = true; >>>> > + else if (Name == "ptx20") >>>> > + Features["ptx20"] = true; >>>> > + else if (Name == "ptx21") >>>> > + Features["ptx21"] = true; >>>> > + else if (Name == "ptx22") >>>> > + Features["ptx22"] = true; >>>> > + else if (Name == "ptx23") >>>> > + Features["ptx23"] = true; >>>> > + else if (Name == "sm10") >>>> > + Features["sm10"] = true; >>>> > + else if (Name == "sm11") >>>> > + Features["sm11"] = true; >>>> > + else if (Name == "sm12") >>>> > + Features["sm12"] = true; >>>> > + else if (Name == "sm13") >>>> > + Features["sm13"] = true; >>>> > + else if (Name == "sm20") >>>> > + Features["sm20"] = true; >>>> > + else if (Name == "sm21") >>>> > + Features["sm21"] = true; >>>> > + else if (Name == "sm22") >>>> > + Features["sm22"] = true; >>>> > + else if (Name == "sm23") >>>> > + Features["sm23"] = true; >>>> > + } else { >>>> > + if (Name == "double") >>>> > + Features["double"] = false; >>>> > + else if (Name == "no-fma") >>>> > + Features["no-fma"] = false; >>>> > + else if (Name == "compute10") >>>> > + Features["compute10"] = false; >>>> > + else if (Name == "compute11") >>>> > + Features["compute11"] = false; >>>> > + else if (Name == "compute12") >>>> > + Features["compute12"] = false; >>>> > + else if (Name == "compute13") >>>> > + Features["compute13"] = false; >>>> > + else if (Name == "compute20") >>>> > + Features["compute20"] = false; >>>> > + else if (Name == "ptx20") >>>> > + Features["ptx20"] = false; >>>> > + else if (Name == "ptx21") >>>> > + Features["ptx21"] = false; >>>> > + else if (Name == "ptx22") >>>> > + Features["ptx22"] = false; >>>> > + else if (Name == "ptx23") >>>> > + Features["ptx23"] = false; >>>> > + else if (Name == "sm10") >>>> > + Features["sm10"] = false; >>>> > + else if (Name == "sm11") >>>> > + Features["sm11"] = false; >>>> > + else if (Name == "sm12") >>>> > + Features["sm12"] = false; >>>> > + else if (Name == "sm13") >>>> > + Features["sm13"] = false; >>>> > + else if (Name == "sm20") >>>> > + Features["sm20"] = false; >>>> > + else if (Name == "sm21") >>>> > + Features["sm21"] = false; >>>> > + else if (Name == "sm22") >>>> > + Features["sm22"] = false; >>>> > + else if (Name == "sm23") >>>> > + Features["sm23"] = false; >>>> > + } >>>> >>>> Does the following code achieve the same? >>>> >>>> std::set<std::string> AvailableFeatures; >>>> AvailableFeatures.add("double"); >>>> AvailableFeatures.add("no-fma"); >>>> AvailableFeatures.add("compute10"); >>>> [...] >>>> >>>> if (AvailableFeatures.count(Name)) >>>> Features[Name] = Enabled; >>>> >>>> You may want to move the AvailableFeatures initialization in the >>>> constructure. >>>> >>>> Cheers >>>> Tobi >>>> >>> >>> I had basically the same remark :) >>> >>> I would however suggest a sorted std::vector<llvm::StringRef> for >>> AvailableFeatures. It provides a lesser memory footprint (about 16 bytes per >>> entry) and offers basically the same complexity for the interface here: >>> std::lower_bound(AvailableFeatures.begin(), AvailableFeatures.end(), Name) >>> != AvailableFeatures.end(); though it'll probably be faster as well as >>> it's got a better cache behavior. >>> >>> Also, it could be possible to keep it close at hand by using a static >>> local variable initialized with a simple function. >>> >> >> Alright, I'll try to fix this by tomorrow. >> > > Fixed in r140320. > > >> >> >>> >>> -- Matthieu >>> >>> _______________________________________________ >>> cfe-commits mailing list >>> >>> [email protected] >>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>> >>> >> >> >> -- >> >> Thanks, >> >> Justin Holewinski >> >> > > > -- > > Thanks, > > Justin Holewinski > > Thanks, it looks much neater.
I only have one remark: you placed a comment "must be in sorted order". I can see two ways to ensure this: > A call to `std::sort`, the vector is small anyway so it should not be too costly > An #ifndef NDEBUG (or equivalent) section to check that it is actually sorted in asserts builds You might not find it's a big deal though, since the number of elements is small and can be encompass in a single glance. -- Matthieu
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
