[llvm-commits] updated patch for PR889

[Chris Lattner]

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