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
_______________________________________________
llvm-commits mailing list
llvm-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
