patch: construct IntrinsicInst hierarchy directly

[Nick Lewycky]

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

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

Right, Instruction * and BasicBlock::iterator are the same thing -- except
that BB.end() returns a BasicBlock::iterator that isn't a valid
Instruction. Whoops.

None of the intrinsics that have wrappers derived from IntrinsicInst take
zero arguments, so I've eliminated those two constructors. I also noticed
that two of CallInst's ctors were dead and eliminated them, and the copies
of them in all the derived IntrinsicInsts. This means that IntrinsicInst
now has 4 constructors, and all the derived wrappers have just 2. This is
starting to look clean.

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

They all friend IntrinsicInst.

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

I agree this is the way that CRTP is normally used. I don't mind CRTP in
the implementation, and there are pieces designed especially for it
(InstVisitor comes to mind), but this is going to exposed in the interface
of LLVM. We have a high bar for keeping this interface a "beginner's"
subset of C++, though exactly what that subset is has been in flux (CRTP is
a well known device these days).

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

I have a strong opinion that using virtual is wrong here and don't want to
contribute to it, but you're right that makes my code the odd one out in
its clone_impl implementation. Changed.

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