[PATCH] Devirtualize llvm::Value and all subclasses
Pete Cooper
peter_cooper at apple.com
Fri Jun 26 13:35:07 PDT 2015
Thanks. I committed this as r240588. Sorry, should have said that at the time.
Regarding 0005, I was again using the pattern of having the same named method in a base class as the subclasses. I’ve fixed that so that now TerminatorInst defines the methods which switch to subclasses, and the subclasses all have an impl.
What do you think?
Cheers,
Pete
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0005.patch
Type: application/octet-stream
Size: 12020 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150626/3ac99eba/attachment.obj>
-------------- next part --------------
> On Jun 24, 2015, at 1:13 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>
> 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