[PATCH] Devirtualize llvm::Value and all subclasses
Duncan P. N. Exon Smith
dexonsmith at apple.com
Tue Jun 23 09:59:29 PDT 2015
> 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.
> //
More information about the llvm-commits
mailing list