llvm dev - Jul 2007 - [LLVMdev] API design (and Boost and tr1)

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

On Wednesday 04 July 2007 14:57, Chris Lattner wrote:
> >  template<typename InputIterator>
> >  CallInst(Value *Func, InputIterator ArgBegin, InputIterator ArgEnd,
> >           const std::string &Name = "", Instruction
*InsertBefore = 0,
>
> Is it acceptable to just make the template argument be the container?
> That way you could pass:
>
> std::vector.. V;
> ...
>
> new CallInst(Callee, V, "foo");
>
> etc.  The disadvantage is that you lose the ability to pass a subrange.
> However, if you know you're using a subrange, you should be able to use
> another ctor form.
I have to think about this a bit but one complication is that clients aren't
always using std-like containers.  There are several places that look like
this:

   Value *array = { v1, v2, v3, v4};
   blah = new CallInst(...,&array[0], 4,...);

Since array doesn't have begin/end members we'd need some kind of 
specialization for that case.  Iterators nicely avoid this issue.

I agree that the SFINAE technique is complicated and esoteric.  I'd rather 
find something more intuitive.  But then again, it is the way "modern
C++" is
heading.  It's actually a quite common technique in C++ generic programming.
So it's not really so out of the mainstream.

                                     -Dave