[PATCH] Devirtualize llvm::Value and all subclasses

[Pete Cooper]

> 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.
> 
>> commit c7dea94fe4be5dc485746b8a83db86dfd04d6bdb
>> Author: Peter Cooper <peter_cooper at apple.com>
>> Date:   Fri Jun 19 19:00:51 2015 -0700
>> 
>>    Create Value.def helper
>> 
>> diff --git a/include/llvm/IR/Value.def b/include/llvm/IR/Value.def
>> new file mode 100644
>> index 0000000..6836ebe
>> --- /dev/null
>> +++ b/include/llvm/IR/Value.def
>> @@ -0,0 +1,90 @@
>> +//===-------- llvm/IR/Value.def - File that describes Values ---v-*- C++ -*-===//
>> +//
>> +//                     The LLVM Compiler Infrastructure
>> +//
>> +// This file is distributed under the University of Illinois Open Source
>> +// License. See LICENSE.TXT for details.
>> +//
>> +//===----------------------------------------------------------------------===//
>> +//
>> +// This file contains descriptions of the various LLVM values.  This is
>> +// used as a central place for enumerating the different values.
>> +//
>> +//===----------------------------------------------------------------------===//
>> +
>> +// NOTE: NO INCLUDE GUARD DESIRED!
>> +
>> +// Provide definitions of macros so that users of this file do not have to 
>> +// define everything to use it...
>> +//
>> +#if !(defined HANDLE_GLOBAL_VALUE || defined HANDLE_CONSTANT ||                \
>> +      defined HANDLE_INSTRUCTION || defined HANDLE_INLINE_ASM_VALUE ||         \
>> +      defined HANDLE_METADATA_VALUE || defined HANDLE_VALUE ||                 \
>> +      defined HANDLE_CONSTANT_MARKER)
>> +#error "Missing macro definition of HANDLE_VALUE*"
>> +#endif
>> +
>> +#ifndef HANDLE_GLOBAL_VALUE
>> +#define HANDLE_GLOBAL_VALUE(ValueName) HANDLE_CONSTANT(ValueName)
>> +#endif
>> +
>> +#ifndef HANDLE_CONSTANT
>> +#define HANDLE_CONSTANT(ValueName) HANDLE_VALUE(ValueName)
>> +#endif
>> +
>> +#ifndef HANDLE_INSTRUCTION
>> +#define HANDLE_INSTRUCTION(ValueName) HANDLE_VALUE(ValueName)
>> +#endif
>> +
>> +#ifndef HANDLE_INLINE_ASM_VALUE
>> +#define HANDLE_INLINE_ASM_VALUE(ValueName) HANDLE_VALUE(ValueName)
> 
> I don't think you need this; see below (don't forget to remove from the
> check at the top).
> 
>> +#endif
>> +
>> +#ifndef HANDLE_METADATA_VALUE
>> +#define HANDLE_METADATA_VALUE(ValueName) HANDLE_VALUE(ValueName)
> 
> Same here.
> 
>> +#endif
>> +
>> +#ifndef HANDLE_VALUE
>> +#define HANDLE_VALUE(ValueName)
>> +#endif
>> +
>> +#ifndef HANDLE_CONSTANT_MARKER
>> +#define HANDLE_CONSTANT_MARKER(MarkerName, ValueName)
>> +#endif
>> +
>> +HANDLE_VALUE(Argument)
>> +HANDLE_VALUE(BasicBlock)
>> +
>> +HANDLE_GLOBAL_VALUE(Function)
>> +HANDLE_GLOBAL_VALUE(GlobalAlias)
>> +HANDLE_GLOBAL_VALUE(GlobalVariable)
>> +HANDLE_CONSTANT(UndefValue)
>> +HANDLE_CONSTANT(BlockAddress)
>> +HANDLE_CONSTANT(ConstantExpr)
>> +HANDLE_CONSTANT(ConstantAggregateZero)
>> +HANDLE_CONSTANT(ConstantDataArray)
>> +HANDLE_CONSTANT(ConstantDataVector)
>> +HANDLE_CONSTANT(ConstantInt)
>> +HANDLE_CONSTANT(ConstantFP)
>> +HANDLE_CONSTANT(ConstantArray)
>> +HANDLE_CONSTANT(ConstantStruct)
>> +HANDLE_CONSTANT(ConstantVector)
>> +HANDLE_CONSTANT(ConstantPointerNull)
>> +
>> +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"
+  }
+
+} 

> 
>> +// 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.

Cheers,
Pete
> 
>> +
>> +HANDLE_CONSTANT_MARKER(ConstantFirstVal, Function)
>> +HANDLE_CONSTANT_MARKER(ConstantLastVal, ConstantPointerNull)
>> +
>> +#undef HANDLE_GLOBAL_VALUE
>> +#undef HANDLE_CONSTANT
>> +#undef HANDLE_INSTRUCTION
>> +#undef HANDLE_METADATA_VALUE
>> +#undef HANDLE_INLINE_ASM_VALUE
>> +#undef HANDLE_VALUE
>> +#undef HANDLE_CONSTANT_MARKER
>> diff --git a/include/llvm/IR/Value.h b/include/llvm/IR/Value.h
>> index 6b36ba6..1a813e2 100644
>> --- a/include/llvm/IR/Value.h
>> +++ b/include/llvm/IR/Value.h
>> @@ -333,32 +333,12 @@ public:
>>   /// Value classes SubclassID field. They are used for concrete type
>>   /// identification.
>>   enum ValueTy {
>> -    ArgumentVal,              // This is an instance of Argument
>> -    BasicBlockVal,            // This is an instance of BasicBlock
>> -    FunctionVal,              // This is an instance of Function
>> -    GlobalAliasVal,           // This is an instance of GlobalAlias
>> -    GlobalVariableVal,        // This is an instance of GlobalVariable
>> -    UndefValueVal,            // This is an instance of UndefValue
>> -    BlockAddressVal,          // This is an instance of BlockAddress
>> -    ConstantExprVal,          // This is an instance of ConstantExpr
>> -    ConstantAggregateZeroVal, // This is an instance of ConstantAggregateZero
>> -    ConstantDataArrayVal,     // This is an instance of ConstantDataArray
>> -    ConstantDataVectorVal,    // This is an instance of ConstantDataVector
>> -    ConstantIntVal,           // This is an instance of ConstantInt
>> -    ConstantFPVal,            // This is an instance of ConstantFP
>> -    ConstantArrayVal,         // This is an instance of ConstantArray
>> -    ConstantStructVal,        // This is an instance of ConstantStruct
>> -    ConstantVectorVal,        // This is an instance of ConstantVector
>> -    ConstantPointerNullVal,   // This is an instance of ConstantPointerNull
>> -    MetadataAsValueVal,       // This is an instance of MetadataAsValue
>> -    InlineAsmVal,             // This is an instance of InlineAsm
>> -    InstructionVal,           // This is an instance of Instruction
>> -    // Enum values starting at InstructionVal are used for Instructions;
>> -    // don't add new values here!
>> +#define HANDLE_VALUE(Name) Name##Val,
>> +#include "llvm/IR/Value.def"
>> 
>>     // Markers:
>> -    ConstantFirstVal = FunctionVal,
>> -    ConstantLastVal  = ConstantPointerNullVal
>> +#define HANDLE_CONSTANT_MARKER(Marker, Constant) Marker = Constant##Val,
>> +#include "llvm/IR/Value.def"
>>   };
>> 
>>   /// \brief Return an ID for the concrete type of this object.
>> 
> 
>