patch: construct IntrinsicInst hierarchy directly

[Nick Lewycky]

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

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.

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.

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

Thanks for looking into this - when I asked about it on IRC I wasn't really
> trying to cause work to happen, just explain some loose ends in the debug
> info. Though this'll only account for13 of my 91 missing types... I might
> hunt for the rest one day.
>
>
> On Tue, Feb 18, 2014 at 9:27 PM, Nick Lewycky <nlewycky at google.com> wrote:
>
>> LLVM's hand-rolled RTTI differs from C++'s in that it allows you to call
>> the construction method on a base class and then based on the dynamic
>> arguments decide to construct a more derived type and return a pointer to
>> that. This looks like calling CallInst::Create and getting back something
>> you can cast<MemSetInst>(). However, we implement this by new'ing a
>> CallInst, which breaks C++ aliasing rules.
>>
>> The attached patch fixes this undefined behaviour by making CallInst
>> forward to IntrinsicInst to construct intrinsics, which in turn uses
>> private constructors on the most derived types. There is some runtime cost,
>> as the implementation of CallInst::Create moved from the header to the .cpp
>> file. If this is a serious problem, let me know and I can probably fix
>> that, I just think the result will result in less much readable code.
>>
>> Please review!
>>
>> As a related issue, I notice that Value has an unnecessary vtable.
>> There's no pressing need to get rid of either form of RTTI (nobody uses
>> dynamic_cast, just normal virtual dispatch), but I want to understand how
>> this happened and whether it's okay to use going forward, or if that
>> happened while nobody was looking and I should go hammer it out of there.
>> It looks like removing it will be a mechanical change.
>>
>> Nick
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140219/aed7a8f4/attachment.html>