[PATCH] Devirtualize llvm::Value and all subclasses
Pete Cooper
peter_cooper at apple.com
Tue Jun 23 15:00:27 PDT 2015
> On Jun 23, 2015, at 9:59 AM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>
>>
>> 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 <mailto: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.
Moved it up there. Thanks for pointing this one out, was no reason to move it.
>
>> - // 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.
Yes, yes it is, and now gone.
Thanks for the review. Committed as r240471.
Pete
>
>> + /// 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.
>> //
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150623/033e9bc0/attachment.html>
More information about the llvm-commits
mailing list