[PATCH] Devirtualize llvm::Value and all subclasses
Duncan P. N. Exon Smith
dexonsmith at apple.com
Wed Jun 24 13:13:26 PDT 2015
LGTM.
> On 2015 Jun 24, at 13:01, Pete Cooper <peter_cooper at apple.com> wrote:
>
> Here’s an updated 0004. This is devirtualizing Instruction::clone_impl.
>
> As I was changing every line anyway, i’ve renamed it to cloneImpl which matches our naming conventions and what Metadata does. Otherwise its mostly a pretty mechanical change.
>
> Cheers,
> Pete
>
> <0004.patch>
>
>> On Jun 24, 2015, at 11:02 AM, Pete Cooper <peter_cooper at apple.com> wrote:
>>
>>>
>>> On Jun 23, 2015, at 6:05 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>>>
>>>
>>>> On 2015-Jun-23, at 17:24, Pete Cooper <peter_cooper at apple.com> wrote:
>>>>
>>>> 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.
>>>
>>> New behaviour makes sense to me. At first blush I thought returning
>>> `nullptr` was worse that returning `this`, but the way you've done it
>>> makes the implementations way simpler, and I guess this pattern matches
>>> lots of other code in LLVM.
>>>
>>>> Perhaps there’s a better name for it at this point?
>>>
>>> Well, it's good that they're named after each other (foo and fooImpl).
>>>
>>> `replaceUsesOfWithOnConstant()` is definitely an eyeful, but it's
>>> somewhat logical that it's based on `replaceUsesOfWith()`. But given
>>> that there's only one user (`replaceUsesOfWith()`), this is probably
>>> good timing to rename it if we ever do.
>>>
>>> We use `handleOperandChange()` in the `Metadata` hierarchy. Since this
>>> *only* gets called during RAUW, I kind of like `handleOperandRAUW()`;
>>> another straw man would be to match `Metadata`'s name exactly. The
>>> "constant" in the name is redundant, since this is API for `Constant`.
>> You’re right about the renaming, especially with a single caller.
>> I was thinking along the lines of ‘replaceUsesOfValue’ but its confusing as we’re replacing operands which is what the Constant is using, not users of the Constant. I prefer handleOperandChange, especially with it being the same as Metadata so i’ll go with that.
>>>
>>> I'm happy for you to just leave well-enough alone, too.
>>>
>>>> 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
>>>>
>>>> <0003.patch>
>>>
>>> One comment below:
>>>
>>>> commit 22105cea3de0994ad53c7868dbefa0122a3a3a44
>>>> Author: Peter Cooper <peter_cooper at apple.com>
>>>> Date: Tue Jun 23 17:19:30 2015 -0700
>>>>
>>>> Devirtualize replaceUsesOfWithOnConstant
>>>>
>>>> diff --git a/include/llvm/IR/GlobalValue.h b/include/llvm/IR/GlobalValue.h
>>>> index 4bca80e..c2a9d7e 100644
>>>> --- a/include/llvm/IR/GlobalValue.h
>>>> +++ b/include/llvm/IR/GlobalValue.h
>>>> @@ -92,7 +92,6 @@ private:
>>>>
>>>> friend class Constant;
>>>> void destroyConstantImpl();
>>>> - void replaceUsesOfWithOnConstant(Value *From, Value *To, Use *U) override;
>>>
>>> Why do it differently for `GlobalValue`? Everywhere else, you define
>>> the `Impl` method in the class, and call `llvm_unreachable()` within it.
>> No particular reason. I did GlobalValue first and was able to handle it with the macro, but for consistency I agree its better to make it a method.
>>>
>>> If there's a good reason for doing it differently (e.g., simplifying a
>>> follow-up patch?), please document that in the commit message.
>>> Otherwise being consistent seems simpler.
>>>
>>> LGTM, whatever you decide.
>> Thanks! I’ll commit shortly with these small changes.
>>
>> Pete
>>>
>>>>
>>>> protected:
>>>> /// \brief The intrinsic ID for this subclass (which must be a Function).
>>>> diff --git a/lib/IR/Constants.cpp b/lib/IR/Constants.cpp
>>>> index 9b2bb27..feb1628 100644
>>>> --- a/lib/IR/Constants.cpp
>>>> +++ b/lib/IR/Constants.cpp
>>>> @@ -2835,7 +2838,27 @@ Constant *ConstantDataVector::getSplatValue() const {
>>>> /// work, but would be really slow because it would have to unique each updated
>>>> /// array instance.
>>>> ///
>>>> -void Constant::replaceUsesOfWithOnConstantImpl(Constant *Replacement) {
>>>> +void Constant::replaceUsesOfWithOnConstant(Value *From, Value *To, Use *U) {
>>>> + Value *Replacement = nullptr;
>>>> + switch (getValueID()) {
>>>> + default:
>>>> + llvm_unreachable("Not a constant!");
>>>> +#define HANDLE_GLOBAL_VALUE(Name) \
>>>> + case Value::Name##Val: \
>>>> + llvm_unreachable("You can't GV->replaceUsesOfWithOnConstant()!");
>>>
>>> ^ Here's the other difference.
>>>
>>>> +#define HANDLE_CONSTANT(Name) \
>>>> + case Value::Name##Val: \
>>>> + Replacement = \
>>>> + cast<Name>(this)->replaceUsesOfWithOnConstantImpl(From, To, U); \
>>>> + break;
>>>> +#include "llvm/IR/Value.def"
>>>> + }
>>>> +
>>>> + // If replaceUsesOfWithOnConstantImpl returned nullptr, then it handled
>>>> + // replacing itself and we don't want to delete or replace anything else here.
>>>> + if (!Replacement)
>>>> + return;
>>>> +
>>>> // I do need to replace this with an existing value.
>>>> assert(Replacement != this && "I didn't contain From!");
>>>>
>>>> diff --git a/lib/IR/Globals.cpp b/lib/IR/Globals.cpp
>>>> index 49ac236..d84b671 100644
>>>> --- a/lib/IR/Globals.cpp
>>>> +++ b/lib/IR/Globals.cpp
>>>> @@ -48,10 +48,6 @@ void GlobalValue::destroyConstantImpl() {
>>>> llvm_unreachable("You can't GV->destroyConstantImpl()!");
>>>> }
>>>>
>>>> -void GlobalValue::replaceUsesOfWithOnConstant(Value *From, Value *To, Use *U) {
>>>> - llvm_unreachable("You can't GV->replaceUsesOfWithOnConstant()!");
>>>> -}
>>>> -
>>>
>>> ^ And here. Seems like you could just rename this.
>>>
>>>> /// copyAttributesFrom - copy all additional attributes (those not needed to
>>>> /// create a GlobalValue) from the GlobalValue Src to this one.
>>>> void GlobalValue::copyAttributesFrom(const GlobalValue *Src) {
>>>>
>>>
>>
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
More information about the llvm-commits
mailing list