On Nov 28, 2007, at 12:55 PM, pawel kunio wrote:
> Hello,
> Here comes updated patch for PR889 with all the changes suggested by
> Chris applied to the source. I was just wondering
> whether the dynamic casts wont impose too much of burden
> performance-wise compared to checking the SubClassID,
> possible open thing for checking the types in Value destructor
> (applies to Instruction inherited classes only).
cast<foo>() is a free conversion. It does a check in an assert build
(which isn't free) but no check in a non-assert build.
dyn_cast<foo>() does a check in every build, but the check just looks
at the subclassid, so it's equivalent to doing a comparison on
subclassid then a C cast (so yeah, it's efficient :)
The patch is looking good, but there are a couple of minor issues:
+ case ConstantExprVal:
+ if(cast<ConstantExpr>(this)->getOpcode() ==
Instruction::GetElementPtr)
+
GetElementPtrConstantExpr
::destroyThis(cast<GetElementPtrConstantExpr>(this));
+ else if(cast<ConstantExpr>(this)->getOpcode() ==
Instruction::ExtractElement)
+
ExtractElementConstantExpr
::destroyThis(cast<ExtractElementConstantExpr>(this));
+ else if(cast<ConstantExpr>(this)->getOpcode() ==
Instruction::InsertElement)
+
InsertElementConstantExpr
::destroyThis(cast<InsertElementConstantExpr>(this));
+ else if(cast<ConstantExpr>(this)->getOpcode() ==
Instruction::Select)
+ SelectConstantExpr::destroyThis(cast<SelectConstantExpr>(this));
+ else if(cast<ConstantExpr>(this)->getOpcode() ==
Instruction::ShuffleVector)
+
ShuffleVectorConstantExpr
::destroyThis(cast<ShuffleVectorConstantExpr>(this));
Please make this fit in 80 columns, for example, by doing:
+ case ConstantExprVal: {
const ConstantExpr *CE = cast<ConstantExpr>(this);
+ if(CE->getOpcode() == Instruction::GetElementPtr)
+
GetElementPtrConstantExpr
::destroyThis(cast<GetElementPtrConstantExpr>(CE));
+ else if(CE->getOpcode() == Instruction::ExtractElement)
+
ExtractElementConstantExpr
::destroyThis(cast<ExtractElementConstantExpr>(CE));
...
+ else if(BinaryConstantExpr* BCE =
dyn_cast<BinaryConstantExpr>(this))
+ BinaryConstantExpr::destroyThis(BCE);
+ else if(UnaryConstantExpr* UCE =
dyn_cast<UnaryConstantExpr>(this))
+ UnaryConstantExpr::destroyThis(UCE);
+ else if(CompareConstantExpr* CCE =
dyn_cast<CompareConstantExpr>(this))
+ CompareConstantExpr::destroyThis(CCE);
It looks like some tabs snuck in here.
+ default:
+ if (BinaryOperator *BO = dyn_cast<BinaryOperator>(this))
+ BinaryOperator::destroyThis(BO);
+ else if (CallInst *CI = dyn_cast<CallInst>(this))
+ {
+ if(IntrinsicInst* II = dyn_cast<IntrinsicInst>(this))
+ {
+ if(DbgInfoIntrinsic* DII =
dyn_cast<DbgInfoIntrinsic>(this))
+ {
+ if(DbgDeclareInst* DDI =
dyn_cast<DbgDeclareInst>(this))
+ DbgDeclareInst::destroyThis(DDI);
+ else if(DbgFuncStartInst* DFSI =
dyn_cast<DbgFuncStartInst>(this))
+ DbgFuncStartInst::destroyThis(DFSI);
+ else if(DbgRegionEndInst* DREI =
dyn_cast<DbgRegionEndInst>(this))
+ DbgRegionEndInst::destroyThis(DREI);
+ else if(DbgRegionStartInst* DRSI =
dyn_cast<DbgRegionStartInst>(this))
+ DbgRegionStartInst::destroyThis(DRSI);
Likewise, more tabs.
+ else if (CmpInst *CI = dyn_cast<CmpInst>(this))
+ {
+ if (FCmpInst *FCI = dyn_cast<FCmpInst>(this))
+ FCmpInst::destroyThis(FCI);
+ else if (ICmpInst *ICI = dyn_cast<ICmpInst>(this))
+ ICmpInst::destroyThis(ICI);
+ else
+ CmpInst::destroyThis(CI);
This pattern occurs frequently, with different classes. This should
only have to handle concrete classes, so I'd prefer that you write
this like:
+ else if (CmpInst *CI = dyn_cast<CmpInst>(this))
+ {
+ if (FCmpInst *FCI = dyn_cast<FCmpInst>(this))
+ FCmpInst::destroyThis(FCI);
+ else if (ICmpInst *ICI = dyn_cast<ICmpInst>(this))
+ ICmpInst::destroyThis(ICI);
+ else
+ assert(0 && "Unknown compare instruction");
or better yet:
+ else if (CmpInst *CI = dyn_cast<CmpInst>(this))
+ {
+ if (FCmpInst *FCI = dyn_cast<FCmpInst>(this))
+ FCmpInst::destroyThis(FCI);
+ else
+ ICmpInst::destroyThis(cast<ICmpInst>(this));
which makes it obvious that the only sort of CmpInst is a FCmpInst or
ICmpInst.
+#if 0
+ else if(isa<User>(this))
+ {
Please remove the #if 0 code.
Otherwise, it looks great!
-Chris
_______________________________________________
llvm-commits mailing list
llvm-commits@cs.uiuc.edu
http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
