patch: construct IntrinsicInst hierarchy directly

[Nick Lewycky]

On 19 February 2014 16:25, David Blaikie <dblaikie at gmail.com> wrote:

>
>
>
> 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.
>

Sorry, I was speaking of clone_impl(), not Create(). I was thinking that
calling clone_impl on Value would handle Argument and a few others directly
by looking at the ValueID, but ValueID is insufficiently specific to decide
how to clone an Instruction, so it would dispatch to
Instruction::clone_impl which would look at the Opcode of the Instruction,
etc. There's another level for intrinsics and intrinsic IDs. Looking
harder, I realize that clone() applies only to the Instruction hierarchy,
not all of Value.

The four Create methods take Function*, optional ArrayRef<Value*> Args,
NameStr, either BasicBlock *InsertAtEnd or Instruction *InsertBefore. A
common way we generalize is to have one overly general form and have
everything else call them, and I think we could forward to a single variant
that has the Args and pass in an empty ArrayRef, but InsertAtEnd and
InsertBefore aren't easily combined (is BasicBlock::end() a valid
Instruction*?).

 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?)
>

Right. Rereading, I need them to go from protected to private, which is
safe to leave as protected. I think inheriting constructors will work in
the C++11 future!

 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
>

IntrinsicInst already derives from CallInst. I'd rather it didn't derive
from both CallInst and also something else -- though I think it would work
in this case, we generally try to avoid it in LLVM.

// 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.
>

Various instantiations of ValueCloneHelper become part of the llvm::Value
hierarchy? Sorry, I don't think this is an "LLVMy" design (I'm arguing
elsewhere that mix-ins are the way to go for the Value hierarchy, but we
aren't there yet). If you want me to change it, I could break up the switch
statement and give each class its own one-line implementation like the rest
of the implementations...

...but when I delete the vtable from Value, I'm going to replace all the
individual clone_impl functions with fewer functions that use switches. I
can defer it until then.

(The plan so far is to expose a public destructor on leaves only (concrete
non-leaves will not expose their destructors since I don't want them to
redo dispatch partway down the chain of destruction), and add a public
destroy() method that will do manual dispatch to the most derived type's
destructor. Subject to change as I actually implement it.)

 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/e020b1e6/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: intrinsic-inst-ctor-2.patch
Type: text/x-patch
Size: 29062 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140219/e020b1e6/attachment.bin>