[PATCH] Devirtualize llvm::Value and all subclasses

[Duncan P. N. Exon Smith]

> 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`.

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.

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.

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