patch: construct IntrinsicInst hierarchy directly

[David Blaikie]

On Wed, Feb 19, 2014 at 3:46 PM, Nick Lewycky <nlewycky at google.com> wrote:

> On 18 February 2014 21:51, David Blaikie <dblaikie at gmail.com> wrote:
>
>> The 3 create functions look like they could be refactored into a
>> template, perhaps? (even just as an implementation detail inside the .cpp
>> file) even clone_impl could use such a thing - but clone_impl might be
>> better implemented via a small CRTP intermediate template class (that way
>> you don't have to do virtual dispatch as well as switching)?
>>
>
> If we remove the vtable on value there would be 3 or so implementations
> all of which use switches internally (Value, Instruction and IntrinsicInst,
> maybe another in Constant somewhere?).
>

Not sure which things you're referring to in that comment, but the 3 I see
initially are IntrinsicInst::Create(..., Instruction*),
IntrinsicInst::Create(..., BasicBlock*), and the 3rd isn't /quite/ the
same, since it takes a 4th argument, an ArrayRef - so without something
more general like perfect forwarding/variadic templates, it wouldn't
generalize.

Generalizing the first two with a template might be nice, but since it
doesn't generalize over all 3 I can see how that's not so
interesting/useful.


>
> The deleted special members in IntrinsicInst still look relevant - was
>> there a reason they needed to be removed?
>>
>
> Instinct to follow the rule of five, since IntrinsicInst now has an
> implicitly defined default constructor that does the right thing. I tried
> adding the deleted members back and everything still works correctly, so I
> assume I should do that.
>

In general everything would "work" because there can't be any callers of
these functions in the current codebase - the concern is if callers would
appear now that those functions can be called and whether they would be
correct. Slicing seems like a reasonable concern and I'd be inclined to
delete the special members at the Instruction level, if not all the way up
at Value.


>
> Having to repeat the same set of ctors in every CallInst is a bit boring -
>> could just template the 4th parameter and forward up to the base?
>> Eventually these could/should be replaced with inheriting ctors, I assume?
>>
>
> I'm not sure. [class.inhctor]/4 "A constructor so declared has the same
> access as the corresponding constructor in X". If this means that I can't
> go from protected to public then that's a problem.
>

Hmm, fair point (alternatively it's probably OK to keep them protected and
just have the derived classes friend IntrinsicInst since the ::Create
functions are the only place that's meant to be building them I assume?)


> We can't use CRTP because we don't want to have multiple bases. I'm
> hopeful that there's some way to compact this, but keep in mind that we
> tend to favour obvious code and we're still using C++98.
>

I wasn't suggesting having multiple bases - something like this:

class IntrinsicInst : ... {
  ...
};


// template<tyepname T> // and when I got to here I realized what you meant
about multiple bases because you have not just one level of derivation, but
things like MemIntrinsic that are intermediate

// OK, so how about this:

template<typename Base, typename Derived>
class ValueCloneHelper : public Base {
protected:
  Derived *clone_impl() const LLVM_OVERRIDE {
    return new Derived(static_cast<const Derived&>(*this));
  }
};

class MemIntrinsic : public ValueCloneHelper<IntrinsicInst, MemIntrinsic> {
};

class MemSetInst : public ValueCloneHelper<MemIntrinsic, MemSetInst> {
};

, etc...

Now, granted, those derived types either have to publicly copy
constructible, which could risk other clients slicing, or they'd have to
friend the ValueCloneHelper base.


>
> I assume the virtua dtors are to create anchors/key functions? Maybe a
>> comment explaining that (or maybe not? it is pretty common in the codebase)
>>
>
> They are. Comments added to the places that didn't have them and the
> destructor was empty.
>

Sounds good.

- Dave
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140219/c414bdfd/attachment.html>