patch: construct IntrinsicInst hierarchy directly

[David Blaikie]

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

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

Hmm, yeah, that's curious. Other containers use the "insert before this
iterator" idiom as the single representation of specified insertion,
including using the "insert before the end iterator" to insert at the end.
I'm not sure if there's something that makes that not possible for BB -
well, I guess the main thing is you're not dealing with iterators, you're
dealing with Instruction *. If you can convert a valid Instruction* to an
iterator trivially, then the underlying operation would take iterators and
the one taking a BB would just pass the end iterator from that and the one
taking the Instruction* would build an iterator from that and pass it.

And, yes, using the zero-length ArrayRef for zero args generalizes over
that extra argument too.


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

OK - haven't fully wrapped my head around what you've got in mind here, but
not sure it's necessary for me to do so in this code review (perhaps we can
just chat about it offline). If they're private in the leaf classes, how
will the base/intermediate class ::Create functions call them?


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

Curious - it's essentially /the/ canonical use of CRTP (which we do employ
elsewhere) and seems ideal here. Admittedly the need for intermediate types
in the hierarchy to be cloneable does add a bit of a wrinkle - on the
flipside, in writing this out I discovered something I'd never actually
written before: a generic CRTP device. It can be reused in any hierarchy
that needs cloning, as long as the clone function is spelled the same way.


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

I'm not incredibly fussed by the difference if it's going to require
repetition - though, yes, I'd tend to err on the side of just using virtual
functions rather than virtual to a switch.


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

Rightio - guess I'll see it when I see it :)

- Dave


>
>  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/98504a75/attachment.html>