[PATCH] Devirtualize llvm::Value and all subclasses

[Duncan P. N. Exon Smith]

> On 2015-Jun-22, at 09:26, Pete Cooper <peter_cooper at apple.com> wrote:
> 
> 
>> On Jun 19, 2015, at 8:49 PM, Duncan P. N. Exon Smith <dexonsmith at apple.com> wrote:
>> 
>> 
>>> On 2015 Jun 19, at 19:03, Pete Cooper <peter_cooper at apple.com> wrote:
>>> 
>>> Here’s a new version of 0001 with all your feedback applied.
>>> 
>>> Will get to the rest next week. Thanks for the feedback so far.
>>> 
>>> Pete
>>> 
>>> <0001.patch>
>> 
>> LGTM with a couple of fixups below.
> Thanks!  Will hold off on committing until you’ve had time to double check my reasoning for the later answers.
>> 
>>> 
>>> +HANDLE_METADATA_VALUE(MetadataAsValue)
>>> +HANDLE_INLINE_ASM_VALUE(InlineAsm)
>> 
>> I think you can just use `HANDLE_VALUE` directly for these, like for
>> `Argument` and `BasicBlock`.
>> 
>>> +
>>> +HANDLE_INSTRUCTION(Instruction)
>> 
>> Actually, why not for `Instruction` too?  Although it has tons of
>> subclasses, it only has the one entry here…
> So all of these were ultimately because the future Value::destroy() method needs to dispatch differently for them compared to constants.  Different Value’s had different behavior so i needed all the different kinds here.  I could actually probably unify Metadata and InlineAsm, but it would be to something cryptic (IMO) like HANDLE_UNDELETABLE_VALUE or something like that.
> 
> Currently the destroy method is in patch 0010 and looks like this.  I’m happy to change all these to HANDLE_VALUE for now and make 0010 make them more specific.  Or if you have a better idea about implementing destroy() then we can make Value.def fit that from the start.
> 
> +void Value::destroy() const {
> +  if (!this) {
> +    // To match the behaviour of delete, we do nothing if null.
> +    return;
> +  }
> +  if (const Instruction *I = dyn_cast<Instruction>(this)) {
> +    I->destroy();
> +    return;
> +  }
> +  switch (getValueID()) {
> +    default:
> +      llvm_unreachable("Illegal value");
> +#define HANDLE_VALUE(Name) \
> +    case Name##Val: \
> +      delete cast<Name>(this); \
> +      return;
> +#define HANDLE_INLINE_ASM_VALUE(Name) \
> +    case Name##Val: \
> +      llvm_unreachable("InlineAsm's shouldn't use Value::destroy()");
> +#define HANDLE_METADATA_VALUE(Name) \
> +    case Name##Val: \
> +      llvm_unreachable("MetadataAsValue's shouldn't use Value::destroy()");
> +#define HANDLE_INSTRUCTION(Name) \
> +    case Name##Val: \
> +      llvm_unreachable("Instructions should never match getValueID()");
> +#include "llvm/IR/Value.def"
> +  }
> +
> +} 

This makes sense.  I don't have any better ideas.  LGTM as is.

> 
>> 
>>> +// Enum values starting at InstructionVal are used for Instructions;
>>> +// don't add new values here!
>> 
>> Out of interest, how hard would it be to get everything into the one
>> .def file?  I don't really understand why subclasses of instruction are
>> stored separately; maybe there's a good reason, but if it's just an
>> historical artifact it would be interesting to know how hard it would
>> be to unify things.
> I can’t think of a reason any more.  Perhaps it was to avoid polluting Value with instructions, but then Value already knows about every other subclass so it now seems strange to leave out instruction.  I’d be happy to move Instruction.def in there, probably after this sequence of patches.

SGTM.