[PATCH] Devirtualize llvm::Value and all subclasses
Pete Cooper
peter_cooper at apple.com
Tue Jun 23 17:24:22 PDT 2015
On to 0003 which is devirtualizing Constant::replaceUsesOfWithOnConstant.
Similarly to 0002, I ended up changing this so that we have Constant::replaceUsesOfWithOnConstant and then all subclasses must implement replaceUsesOfWithOnConstantImpl.
Given the way this code was structured, I ended up having to make replaceUsesOfWithOnConstantImpl return either a value to replace with, or nullptr if no replacement should be done. Perhaps there’s a better name for it at this point?
Constant:: replaceUsesOfWithOnConstant then does as expected and dispatches to the appropriate subclass to get a replacement, then if it gets a replacement, deletes the current value after replacing uses with the new value.
Cheers,
Pete
> On Jun 23, 2015, at 3:00 PM, Pete Cooper <peter_cooper at apple.com> wrote:
>
>
>> On Jun 23, 2015, at 9:59 AM, Duncan P. N. Exon Smith <dexonsmith at apple.com <mailto:dexonsmith at apple.com>> wrote:
>>
>>>
>>> On 2015-Jun-22, at 17:53, Pete Cooper <peter_cooper at apple.com <mailto: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.
>>> //
>
> _______________________________________________
> 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/20150623/609d33b5/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003.patch
Type: application/octet-stream
Size: 18563 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150623/609d33b5/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150623/609d33b5/attachment-0001.html>
More information about the llvm-commits
mailing list