[PATCH] Devirtualize llvm::Value and all subclasses
Pete Cooper
peter_cooper at apple.com
Wed Jun 24 13:01:51 PDT 2015
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
> 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 <mailto:llvm-commits at cs.uiuc.edu>
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits <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/20150624/a3f98ab6/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0004.patch
Type: application/octet-stream
Size: 27747 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150624/a3f98ab6/attachment.obj>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150624/a3f98ab6/attachment-0001.html>
More information about the llvm-commits
mailing list