patch: construct IntrinsicInst hierarchy directly

[David Blaikie]

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

The deleted special members in IntrinsicInst still look relevant - was
there a reason they needed to be removed?

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

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/20140218/6aa7e9d4/attachment.html>