[llvm-commits] [llvm] r75739 - in /llvm/trunk: include/llvm/Target/TargetRegistry.h lib/Support/TargetRegistry.cpp

[Chris Lattner]

On Jul 14, 2009, at 9:25 PM, Daniel Dunbar wrote:
> Author: ddunbar
> Date: Tue Jul 14 23:24:58 2009
> New Revision: 75739
>
> URL: http://llvm.org/viewvc/llvm-project?rev=75739&view=rev
> Log:
> Add new TargetRegistry.

Yay, nice.  This should ultimately lead to major simplifications.

> +  /// Target - Wrapper for Target specific information.
> +  ///
> +  /// For registration purposes, this is a POD type so that targets  
> can be
> +  /// registered without the use of static constructors.

Please comment that instances of this should be declared as global  
variables, and that this depends on them being zero initialized.

> +  struct Target {

How about something like this:

struct Target {
   bool (*MatchPredicate)();
};

template<typename T>
struct TargetImpl : public Target {
   TargetImpl() {
     MatchPredicate = T::TargetMatchPredicate;
   }
};

struct X86Target : public TargetImpl<X86Target> {
   static bool TargetMatchPredicate() {
     return false;
   };
} TheX86Target;

Doing something like this will allow static initialization (llvm-gcc  
will evaluate the static ctors) but be a bit nicer than having to pass  
everything around explicitly.

> +    /// TripleMatchQualityFn - The target function for rating the  
> match quality
> +    /// of a triple.
> +    TripleMatchQualityFnTy TripleMatchQualityFn;
> +
> +    /// ModuleMatchQualityFn - The target function for rating the  
> match quality
> +    /// of a module.
> +    ModuleMatchQualityFnTy ModuleMatchQualityFn;

Instead of returning a random unsigned, this might be a good time to  
introduce an enum for "strong match", "weak match" etc.   
Alternatively, we could just go for "strong match" or "no match".   
There is code that tries to do a fuzzy match based on the target data  
if a triple is not around, this is old and baroque and probably should  
be ripped out.

> +  /// TargetRegistry - Generic interface to target specific features.
> +  //
> +  // FIXME: Provide Target* iterator.
> +  struct TargetRegistry {

Please make this a class with all public members.  This simplifies  
forward declaration for VC++.

> +    /// @name Registry Access
> +    /// @{
> +
> +    /// getClosestStaticTargetForTriple - Given a target triple,  
> pick the most
> +    /// capable target for that triple.
> +    static const Target *getClosestStaticTargetForTriple(const  
> std::string &TT,
> +                                                          
> std::string &Error);
> +

> +    /// getClosestStaticTargetForModule - Given an LLVM module,  
> pick the best
> +    /// target that is compatible with the module.  If no close  
> target can be
> +    /// found, this returns null and sets the Error string to a  
> reason.
> +    static const Target *getClosestStaticTargetForModule(const  
> Module &M,
> +                                                        std::string  
> &Error);

This function could be tricky.  We don't want TargetRegistry.cpp to  
depend on VMCore.  This can be worked around by making the part that  
touches Module be inline, and call into an out-of-line version that  
takes the guts of module (e.g. the triple from it).  Alternatively, we  
could just eliminate this method.

> +    /// RegisterTarget - Register the given target.
> +    ///
> +    /// @param T - The target being registered.
> +    /// @param Name - The target name. This should be a static  
> string.
> +    /// @param ShortDesc - A short target description. This should  
> be a static
> +    /// string.
> +    /// @param TQualityFn - The triple match quality computation  
> function for
> +    /// this target.
> +    /// @param MQualityFn - The module match quality computation  
> function for
> +    /// this target.
> +    /// @param JITMatchQualityFn - The JIT match quality  
> computation function
> +    /// for this target.
> +    static void RegisterTarget(Target &T,
> +                               const char *Name,
> +                               const char *ShortDesc,
> +                               Target::TripleMatchQualityFnTy  
> TQualityFn,
> +                               Target::ModuleMatchQualityFnTy  
> MQualityFn,
> +                               Target::JITMatchQualityFnTy  
> JITQualityFn);

Using a template like above would remove some of these, which would be  
nice.  It would be nice for the target to just define its entry point  
like:

extern "C" void InitializeX86Target() {
   TargetRegistry::RegisterTarget(TheX86Target);
}

> +
> +    /// RegisterTargetMachine - Register a TargetMachine  
> implementation for the
> +    /// given target.
> +    ///
> +    /// @param T - The target being registered.
> +    /// @param Fn - A function to construct a TargetMachine for the  
> target.
> +    static void RegisterTargetMachine(Target &T,
> +                                      Target::TargetMachineCtorTy  
> Fn) {
> +      assert(!T.TargetMachineCtorFn && "Constructor already  
> registered!");
> +      T.TargetMachineCtorFn = Fn;
> +    }
> +
> +    /// RegisterAsmPrinter - Register an AsmPrinter implementation  
> for the given
> +    /// target.
> +    ///
> +    /// @param T - The target being registered.
> +    /// @param Fn - A function to construct an AsmPrinter for the  
> target.
> +    static void RegisterAsmPrinter(Target &T,  
> Target::AsmPrinterCtorTy Fn) {
> +      assert(!T.TargetMachineCtorFn && "Constructor already  
> registered!");
> +      T.AsmPrinterCtorFn = Fn;
> +    }

Makes sense.


> +port/TargetRegistry.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Support/TargetRegistry.cpp?rev=75739&view=auto
>
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- llvm/trunk/lib/Support/TargetRegistry.cpp (added)
> +++ llvm/trunk/lib/Support/TargetRegistry.cpp Tue Jul 14 23:24:58 2009
> @@ -0,0 +1,136 @@
> +//===--- TargetRegistry.cpp - Target registration  
> -------------------------===//
> +//
> +//                     The LLVM Compiler Infrastructure
> +//
> +// This file is distributed under the University of Illinois Open  
> Source
> +// License. See LICENSE.TXT for details.
> +//
> +// 
> = 
> = 
> = 
> ----------------------------------------------------------------------= 
> ==//
> +
> +#include "llvm/Target/TargetRegistry.h"
> +#include <cassert>
> +using namespace llvm;
> +
> +// FIXME: Worry about locking? In general everything should be  
> registered at
> +// startup.

Just document the registration methods as having to be called before  
llvm_start_multithreading happens.

>
> +const Target *
> +TargetRegistry::getClosestStaticTargetForModule(const Module &M,
> +                                                std::string &Error) {
> +  Target *Best = 0, *EquallyBest = 0;
> +  unsigned BestQuality = 0;
> +  // FIXME: Use iterator.
> +  for (Target *i = FirstTarget; i; i = i->Next) {
> +    if (unsigned Qual = i->ModuleMatchQualityFn(M)) {

Ah, so this doesn't depend on vmcore, but it forces the TargetInfo  
implementations to depend on vmcore.  That's not nice either :).

Looks like a great step!

-Chris