[llvm-commits] PR889 patch

[Chris Lattner]

On Nov 26, 2007, at 4:46 AM, pawel kunio wrote:

> Hello,
> Please find attached the changelist for PR889. As suggested in
> bugzilla, I devirtualized the Value destructor, renamed the
> destructors of Value-inherited classes to static destroythis methods,
> added the code for type checking and destroythis scheduling to Value
> destructor.
> Currently i am not full-time available for this project but I will try
> to get some new tasks on llvmdev mailing list, as time allows.

Very nice work Pawel!

Some specific comments:

--- include/llvm/BasicBlock.h	(revision 44322)
+++ include/llvm/BasicBlock.h	(working copy)
@@ -76,7 +76,7 @@
    ///
    explicit BasicBlock(const std::string &Name = "", Function *Parent  
= 0,
                        BasicBlock *InsertBefore = 0);
-  ~BasicBlock();
+  static void destroythis(BasicBlock*);


Can this and other instances of 'destroyThis' be made private or  
protected?  I think only the ~Value() method should be public.  This  
may require making Value a friend of everything, but that is less  
confusing than having this as a public method.



Index: include/llvm/Constants.h
===================================================================
--- include/llvm/Constants.h	(revision 44322)
+++ include/llvm/Constants.h	(working copy)
@@ -24,6 +24,7 @@
  #include "llvm/Type.h"
  #include "llvm/ADT/APInt.h"
  #include "llvm/ADT/APFloat.h"
+#include "llvm/Instructions.h"


Please don't add this #include.  Instead, use forward declarations and  
move code that needs it out of line (I think that moving the  
GetElementPtrConstantExpr ctor out of line would be enough?)

+/// GetElementPtrConstantExpr - Helper class for Constants.cpp, and is
+/// used behind the scenes to implement getelementpr constant exprs.
+struct GetElementPtrConstantExpr : public ConstantExpr {
+  GetElementPtrConstantExpr(Constant *C, const std::vector<Constant*>  
&IdxList,
+                            const Type *DestTy)

Please define this as a 'class' and use "public" instead of a  
'struct'.  This improves compatibility with MSVC++.


--- include/llvm/InlineAsm.h	(revision 44322)
+++ include/llvm/InlineAsm.h	(working copy)
@@ -36,7 +36,8 @@

    InlineAsm(const FunctionType *Ty, const std::string &AsmString,
              const std::string &Constraints, bool hasSideEffects);
-  virtual ~InlineAsm();
+  //static void destroythis(InlineAsm*);
+  //friend Value;
  public:


Please remove :), like wise in UnaryInstruction.

+++ include/llvm/Value.h	(working copy)
@@ -64,14 +64,17 @@

    friend class ValueSymbolTable; // Allow ValueSymbolTable to  
directly mod Name.
    friend class SymbolTable;      // Allow SymbolTable to directly  
poke Name.
-  ValueName *Name;
+public:
+	ValueName *Name;


Why does this need to be public?  If it really needs to be public,  
please add a comment explaining why.  Also, please remove the tab.


+void ConstantArray::destroythis(ConstantArray*v)
+{
+  delete [] v->OperandList;
  }

Unlike destructors, we don't get free "Chaining" of destruction up  
through parent classes for free.  I think it would be better to  
explicitly model the class hierarchy here, with something like:

+void ConstantArray::destroythis(ConstantArray*v)
+{
+  delete [] v->OperandList;
    Constant::destroyThis(v);
  }

And implement destroyThis for every class (even if it's a trivial  
empty forwarding method that just calls destroyThis on its immediate  
parent class).  What do you think?

+Value::~Value()
+{
+	if(SubclassID == BasicBlockVal)
+	{
+		BasicBlock::destroythis((BasicBlock*)this);
+	}
+	else if(SubclassID == ConstantArrayVal)
+	{

Why not use a switch on subclassid?

+	else if(SubclassID == ConstantExprVal && ((ConstantExpr*)this)- 
 >SubclassData == Instruction::GetElementPtr)

I'd much prefer that this look like this:

     case ConstantExprVal:
       if (cast<ConstantExpr>(this)->getOpcode() ==  
Instruction::GetElementPtr)
         ...

+	else if(SubclassID >= InstructionVal)
+	{
+		if(SubclassID == InstructionVal + Instruction::Call)
+		{
+			CallInst::destroythis((CallInst*)this);
+		}
+		else if(SubclassID == InstructionVal + Instruction::PHI)


Likewise, this can be:

    default:
      if (CallInst *CI = dyn_cast<CallInst>(this))
        CallInst::destroyThis(CI);
      else if (PHINode *PN = dyn_cast<PHINode>(this))
        PHINode::destroyThis(PN);
       ..
      else
        Instruction::destroyThis(PN);

I really think it is cleaner if CallInst::destroyThis itself chains to  
Instruction::destroyThis, instead of having ~Value do it.

Thanks for tackling this project, it will be very nice to shrinkify  
everything!

-Chris