[PATCH] Devirtualize llvm::Value and all subclasses

[Duncan P. N. Exon Smith]

> On 2015-Jun-22, at 17:53, Pete Cooper <peter_cooper at apple.com> wrote:
> 
> 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

LGTM with a little nitpicking.

> commit faa9ec17ac16b8b0c4f22c16d353e0ee13590253
> Author: Peter Cooper <peter_cooper at apple.com>
> Date:   Mon Jun 15 13:04:29 2015 -0700
> 
>     Devirtualize Constant::destroyConstant.
>     
>     This reorganizes destroyConstant and destroyConstantImpl.
>     
>     Now there is only destroyConstant in Constant itself, while
>     subclasses are required to implement destroyConstantImpl.
>     
>     destroyConstantImpl no longer calls delete but is instead only
>     responsible for removing the constant from any maps in which it
>     is contained.
> 
> diff --git a/lib/IR/Constants.cpp b/lib/IR/Constants.cpp
> index 76c55b6..773c829 100644
> --- a/lib/IR/Constants.cpp
> +++ b/lib/IR/Constants.cpp
> @@ -276,35 +276,6 @@ Constant *Constant::getAggregateElement(Constant *Elt) const {
>    return nullptr;
>  }
>  
> -
> -void Constant::destroyConstantImpl() {

Can you move `Constant::destroyConstant()` up to here to minimize the
diff?  Alternatively, if there's some reason to move the code, please do
it in a separate NFC commit.

> -  // When a Constant is destroyed, there may be lingering
> -  // references to the constant by other constants in the constant pool.  These
> -  // constants are implicitly dependent on the module that is being deleted,
> -  // but they don't know that.  Because we only find out when the CPV is
> -  // deleted, we must now notify all of our users (that should only be
> -  // Constants) that they are, in fact, invalid now and should be deleted.
> -  //
> -  while (!use_empty()) {
> -    Value *V = user_back();
> -#ifndef NDEBUG      // Only in -g mode...
> -    if (!isa<Constant>(V)) {
> -      dbgs() << "While deleting: " << *this
> -             << "\n\nUse still stuck around after Def is destroyed: "
> -             << *V << "\n\n";
> -    }
> -#endif
> -    assert(isa<Constant>(V) && "References remain to Constant being destroyed");
> -    cast<Constant>(V)->destroyConstant();
> -
> -    // The constant should remove itself from our use list...
> -    assert((use_empty() || user_back() != V) && "Constant not removed!");
> -  }
> -
> -  // Value has no outstanding references it is safe to delete it now...
> -  delete this;
> -}
> -
>  static bool canTrapImpl(const Constant *C,
>                          SmallPtrSetImpl<const ConstantExpr *> &NonTrappingOps) {
>    assert(C->getType()->isFirstClassType() && "Cannot evaluate aggregate vals!");
> @@ -1432,6 +1409,45 @@ const APInt &Constant::getUniqueInteger() const {
>    return cast<ConstantInt>(C)->getValue();
>  }
>  
> +void Constant::destroyConstant() {
> +

This newline is strange.

> +  /// First call destroyConstantImpl on the subclass.  This gives the subclass
> +  /// a chance to remove the constant from any maps/pools it's contained in.
> +  switch (getValueID()) {
> +  default:
> +    llvm_unreachable("Not a constant!");
> +#define HANDLE_CONSTANT(Name)                                                  \
> +  case Value::Name##Val:                                                       \
> +    return cast<Name>(this)->destroyConstantImpl();
> +#include "llvm/IR/Value.def"
> +  }
> +
> +  // When a Constant is destroyed, there may be lingering
> +  // references to the constant by other constants in the constant pool.  These
> +  // constants are implicitly dependent on the module that is being deleted,
> +  // but they don't know that.  Because we only find out when the CPV is
> +  // deleted, we must now notify all of our users (that should only be
> +  // Constants) that they are, in fact, invalid now and should be deleted.
> +  //
> +  while (!use_empty()) {
> +    Value *V = user_back();
> +#ifndef NDEBUG // Only in -g mode...
> +    if (!isa<Constant>(V)) {
> +      dbgs() << "While deleting: " << *this
> +             << "\n\nUse still stuck around after Def is destroyed: " << *V
> +             << "\n\n";
> +    }
> +#endif
> +    assert(isa<Constant>(V) && "References remain to Constant being destroyed");
> +    cast<Constant>(V)->destroyConstant();
> +
> +    // The constant should remove itself from our use list...
> +    assert((use_empty() || user_back() != V) && "Constant not removed!");
> +  }
> +
> +  // Value has no outstanding references it is safe to delete it now...
> +  delete this;
> +}
>  
>  //---- ConstantPointerNull::get() implementation.
>  //