[LLVMdev] API design (and Boost and tr1)

[David A. Greene]

On Monday 02 July 2007 23:24, David A. Greene wrote:

> > > - Changing the API
> > >  a) Template it to take two iterators. This causes code size bloat.
>
> This seems like the right solution to me.  Unless llvm is running on
> extremely limited memory embedded systems, the extra code space
> shouldn't be an issue.

It turns out this wasn't quite a simple as we'd thought.  The problem is
the CallInst constructors that take two Value * arguments.  They look
just like the teamplte versions that take iterators.

Unfortunately, there is at least one place in llvm where a CallInst is
constructed with two AllocaInst pointers.  Because the template constructor
is a better match, it is selected over the Value * constructors.

To get around this problem, I've used SFINAE to remove the template 
constructors from the overload set when passed pointers to classes derived
from Value:

  namespace detail {
    template<bool value>
    struct disable_if {};

    template<>
    struct disable_if<false> {
      typedef int type;
    };
  }

  /// CallInst - This class represents a function call, abstracting a target
  [...]

  /// Construct a CallInst given a range of arguments.  InputIterator
  /// must be a random-access iterator pointing to contiguous storage
  /// (e.g. a std::vector<>::iterator).  Checks are made for
  /// random-accessness but not for contiguous storage as that would
  /// incur runtime overhead.
  /// @brief Construct a CallInst from a range of arguments
  template<typename InputIterator>
  CallInst(Value *Func, InputIterator ArgBegin, InputIterator ArgEnd,
           const std::string &Name = "", Instruction *InsertBefore = 0,
           // This extra default argument use SFINAE to prevent the
           // template from being considered for pointers to
           // Value-derived types.  It removes an unwanted
           // instantiation when the caller really wants one of the
           // Value * versions below.  Because Value is a base class,
           // the template match is preferred for classes derived from
           // Value if this argument is not here.  Note that if
           // InputIterator IS Value *, the non-template constructor
           // is a better match.
           typename detail::disable_if<
             is_base_and_derived<
               Value,
               typename remove_pointer<InputIterator>::type
             >::value
           >::type enabler = 0)
        : Instruction(cast<FunctionType>(cast<PointerType>(Func->getType())
                                         ->getElementType())->getReturnType(),
                      Instruction::Call, 0, 0, InsertBefore) {
    init(Func, ArgBegin, ArgEnd, Name,
         typename std::iterator_traits<InputIterator>::iterator_category());
  }

init() is the function that actually does the checking of the iterators for
an empty range.

This required that I pull in is_base_and_derived, remove_pointer, is_same and
remove_cv from Boost.  boost::is_class already exists in Support/type_traits.h
so there is precedent for this.

Now, something is not quite right with the code as the overload doesn't seem
to get removed when passed an AllocaInst *.  I'm working on figuring out why.
I wrote a small testcase which passes.  I'm working on reducing 
UpgradeParser.cc to a minimal testcase that shows the problem.  I'm wondering
if I've stumbled on a gcc bug.

Is this an acceptable solution to the problem?  Is importing these Boost 
components ok?  tr1 includes all of the Boost type traits so another option
is to just use those and impose a requirement that compilers used to build
llvm must support tr1 type traits.  GNU libstdc++ already does.

                                    -Dave