patch: construct IntrinsicInst hierarchy directly

[Nick Lewycky]

On 19 February 2014 10:11, Philip Reames <listmail at philipreames.com> wrote:

>
> On 02/18/2014 09:27 PM, Nick Lewycky 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!
>>
> My only correctness concern would be the removal of:
> void operator=(const IntrinsicInst&) LLVM_DELETED_FUNCTION;
> from IntrinsicInst.  That looks suspicious.
>
> I agree with David on the yuck factor of that much duplicated code though.
>  Templates, or other copy removal would be appreciated, but not required.
>
> Another point: is there any way to detect if someone adds a new subclass
> of Instrinsic which needs added to your switch?  Maybe an assert in the
> protected constructor which is only satisfied coming from Create(...)?
>  Seems a bit hacky, but might save someone a lot of debugging time.  Again,
> appreciated, not required.
>

I don't think that would work either? IntrinsicInst is sometimes the
dynamic type. If somebody adds a new type, and doesn't update the switch in
Create then we end up falling through to creating an IntrinsicInst instead
of their new type. We can't assert on that because we can't tell whether
this intrinsic is supposed to be created as an IntrinsicInst or if there
happens to exist a wrapper type we needed to use.

Nick
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140219/d9b0fa06/attachment.html>