[PATCH] Devirtualize llvm::Value and all subclasses

[Pete Cooper]

So back to 0002.

You were worried about the lack of protection to ensure that a subclass of Constant actually implements destroyConstant.  Unfortunately I couldn’t find a good way to check for this statically, but I did go ahead and implement the alternative you suggested.  For context, what follows (indented) was the proposal which i’ve basically just implemented exactly as described.  It seems to be a very good way to handle this, and isn’t much more code churn.

However, I realize there's already something called
`Constant::destroyConstantImpl()`, which has a `delete this` inside it
(ironic, given that the context for this patch is removing the vtable
that the `delete` call relies on).

I haven't looked at all the patches in this series yet, but I feel like
there ought to be some way of clarifying `Constant` destruction
immediately.  My shot-from-the-hip is something like the following:

   class Constant {
   public:
     void destroyConstant();
   };

   class SomeConstant : public Constant {
     friend class Base; // For fooImpl().

     /// Destroy and delete the constant.
     void destroyConstantImpl();
     ~SomeConstant();

     // Don't provide destroyConstant().
   };

   void Constant::destroyConstant() {
     // Remove lingering references from the constant pool (move from
     // old `Constant::destroyConstantImpl()`).
     while (!use_empty()) {
       // ...
     }

     // Dispatch to subclass to cleanup and delete.
     switch (...) {
     default:
       llvm_unreachable(...);
     // Compile error if there's an unhandled case instead of
     // infinite recursion.
   #define HANDLE_CONSTANT(NAME)                       \
     case NAME ## Kind:                                \
       cast<NAME>(this)->destroyConstantImpl();        \
       break;
     }

     // When we drop virtual dispatch for the destructor, move the
     // delete call inside the switch statement above.
     delete this;
   }

   void SomeConstant::destroyConstantImpl() {
     assert(use_empty() && ...);
     getContext()->SomeConstantPool.erase(this);
   }

This inverts the destroyConstant/Impl relationship.

Maybe this leaves out some case, or doesn't quite fit with the end goal
(you've thought about this more than I have).  My main point is, with
static dispatch we can easily catch the "missing destroyConstant()
implementation" at compile-time, and we should.

Cheers,
Pete

> On Jun 22, 2015, at 5:03 PM, Pete Cooper <peter_cooper at apple.com> wrote:
> 
> 
>> On Jun 22, 2015, at 10:47 AM, Duncan P. N. Exon Smith <dexonsmith at apple.com <mailto:dexonsmith at apple.com>> wrote:
>> 
>> This makes sense.  I don't have any better ideas.  LGTM as is.
> Thanks.  Landed 0001 as r240358.
> 
> Starting to look at the next one.  Trying to work out whether I can statically tell if A::foo != B::foo so that we know methods have been overridden.  So far no luck but i’ll keep working on it.
> 
> Cheers,
> Pete
> 
> _______________________________________________
> 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/20150622/2472fb1a/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002.patch
Type: application/octet-stream
Size: 19134 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150622/2472fb1a/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150622/2472fb1a/attachment-0001.html>