[PATCH] Devirtualize llvm::Value and all subclasses

[Pete Cooper]

> 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) {
>> 
>