[PATCH] D31261: [IR] De-virtualize ~Value to save a vptr

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc requested changes to this revision.
chandlerc added a comment.
This revision now requires changes to proceed.

Have you checked Polly and Clang for changes needed there? Detailed comments below.



================
Comment at: llvm/include/llvm/IR/DerivedUser.h:10-14
+// This file declares the DerivedUser class, which is an extension point for the
+// Value class hierarchy. Generally speaking, Value is the base of a closed
+// class hierarchy that can't be extended by code outside of lib/IR. This class
+// creates a loophole that allows classes outside of lib/IR to extend User to
+// leverage its use/def list machinery.
----------------
Given that this header just defines a class, maybe just move all the comments to the class doxygen and nuke the intro section?


================
Comment at: llvm/include/llvm/IR/DerivedUser.h:27-29
+/// Extension point for the Value hierarchy. All classes outside of lib/IR
+/// that wish to inherit from User should instead inherit from DerivedUser
+/// instead.
----------------
I'd personally like some moderately strong discouragement of using this and indicate that if we find another way to extend use/def chains it may go away. I just don't want folks outside the project designing things on top of this extension point and then being in a really bad spot if we ever get rid of it.


================
Comment at: llvm/include/llvm/IR/DerivedUser.h:42
+/// lib/IR.
+class ValueCallbacks {
+public:
----------------
george.burgess.iv wrote:
> rnk wrote:
> > pcc wrote:
> > > Can "ValueCallbacks" just be a function pointer?
> > It could be, but I had the idea that MemorySSA might prefer to add virtual methods here instead of doing switch dispatch like I ended up doing. Whatever dannyb/gbiv prefer, I'll do that.
> We don't have that many virtual methods in Memory*, so I'd be perfectly happy with switch dispatch. If that changes, looks straightforward to swap back to this approach.
> 
> (If we take the `ValueCallbacks` route, though, please make `deleteValue` and `VCallbacks` const)
+1 for me FWIW.


================
Comment at: llvm/include/llvm/IR/GlobalValue.h:179
 
-  ~GlobalValue() override {
-    removeDeadConstantUsers();   // remove any dead constants using this.
-  }
-
+public:
   unsigned getAlignment() const;
----------------
Redundant public.


================
Comment at: llvm/include/llvm/IR/Instruction.def:192-193
 HANDLE_OTHER_INST(55, Select , SelectInst )  // select instruction
-HANDLE_OTHER_INST(56, UserOp1, Instruction)  // May be used internally in a pass
-HANDLE_OTHER_INST(57, UserOp2, Instruction)  // Internal to passes only
+HANDLE_USER_INST(56, UserOp1, Instruction)  // May be used internally in a pass
+HANDLE_USER_INST(57, UserOp2, Instruction)  // Internal to passes only
 HANDLE_OTHER_INST(58, VAArg  , VAArgInst  )  // vaarg instruction
----------------
I guess you should match the weird formatting of this file for this patch. We should probably come back and clean up all of this formatting ,but seems weird to deviate here.


================
Comment at: llvm/include/llvm/IR/Instruction.h:59
+protected:
+  ~Instruction(); // Use deleteValue() to delete a generic Instruction.
 
----------------
Make this a doxygen comment?


================
Comment at: llvm/include/llvm/IR/User.h:96
   User(const User &) = delete;
-  ~User() override = default;
+protected:
+  ~User() = default;
----------------
Add whitespace before the protected?


================
Comment at: llvm/include/llvm/IR/Value.def:61
 HANDLE_VALUE(BasicBlock)
-HANDLE_VALUE(MemoryUse)
-HANDLE_VALUE(MemoryDef)
-HANDLE_VALUE(MemoryPhi)
+
+HANDLE_MEMORY_VALUE(MemoryUse)
----------------
Leave a FIXME comment here to document that this isn't ideal but we don't have any better idea?


================
Comment at: llvm/include/llvm/IR/Value.h:209-214
+  /// Value's destructor should be virtual by design, but that would require
+  /// that Value and all of its subclasses have a vtable that effectively
+  /// duplicates the information in the value ID. As a size optimization, the
+  /// destructor has been protected, and the caller should manually call
+  /// deleteValue.
+  ~Value(); // Use deleteValue() to delete a generic Value.
----------------
Move this up to the protected region with the constructor?

Also, the trailing comment doesn't seem to add value at this point.


================
Comment at: llvm/include/llvm/IR/Value.h:216
+public:
+
+  /// Delete a pointer to a generic Value.
----------------
No blank line here.


https://reviews.llvm.org/D31261