[llvm-commits] PR889 patch

Chris Lattner clattner at apple.com
Mon Nov 26 22:31:51 PST 2007

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*>  
+                            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;

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;
+	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;

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?

+	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() ==  

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

Likewise, this can be:

      if (CallInst *CI = dyn_cast<CallInst>(this))
      else if (PHINode *PN = dyn_cast<PHINode>(this))

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  



More information about the llvm-commits mailing list