[LLVMdev] Problems in removing a cloned instruction.

[Matthijs Kooijman]

Hi,

I'm gonna try to give some feedback, but I have only been working with LLVM
for a few days, so don't take what I'm saying without verifying :-)

> BasicBlock *ProgSlicer::CloneBasicBlock(const BasicBlock *BB,
>                   DenseMap<const Value*, Value*> &ValueMap,
>                   const char *NameSuffix, Function *F) {
> 
>         BasicBlock *NewBB = new BasicBlock("", F);
>           if (BB->hasName()) NewBB->setName(BB->getName()+NameSuffix);
> 
>           bool hasCalls = false, hasDynamicAllocas = false,
> hasStaticAllocas = false, isTerminal =false;
> 
>   // Loop over all instructions, and copy them over.
>           for (BasicBlock::const_iterator II = BB->begin(), IE = BB->end();
>                     II != IE; ++II)
>           {
>             const Instruction *NwInst = cast<Instruction>((II));
What's this for? You don't seem to use it anywhere?

>             Instruction *NewInst = II->clone();
Why do you clone the instruction here already? Can't you wait until you know
that II is not a ReturnInst? AFAIK you can simply dyn_cast II instead of
NewInst in the next line. This saves you from having to cleanup NewInst below
if you're not going to use it.

>             if(ReturnInst *RI = dyn_cast<ReturnInst>(NewInst))
>             {
>                 cerr << "\n Return Found : "  << *RI<< "\n";
> 
>                 Instruction *NewRInst = new ReturnInst();  // creating a new return instruction
>                 Value *v = NULL;
> 
>                 if (II->hasName())
>                     NewRInst->setName(II->getName()+NameSuffix);
>                 NewBB->getInstList().push_back(NewRInst);
>                 ValueMap[II] = NewRInst;                // Add instruction map to value.
>                 hasCalls |= isa<CallInst>(II);
Can this one ever be true? Intuitively, if II is a ReturnInst (which is the
case inside this if), it can't be a CallInst as well?
>             }
> 
>                 if (II->hasName())
>                 NewInst->setName(II->getName()+NameSuffix);
>                 NewBB->getInstList().push_back(NewInst);
>                 ValueMap[II] = NewInst;                // Add instruction map to value.
>                     hasCalls |= isa<CallInst>(II);
Do you really need to do all this if II is a ReturnInst? Shouldn't this block
be in the else of the if(ReturnInst... above?

> 
>             if(ReturnInst *RI = dyn_cast<ReturnInst>(NewInst)) // deleting the duplicate return
>             {
>                 cerr << "\n INST : "  << *NewInst << " Par : " << *(NewInst->getParent()) << "\n";
>                 NewInst->eraseFromParent();
Is this really enough cleanup? AFAICS, this will at least present a memory
leak (since NewInst is a pointer, the cloned instruction will not have it's
destructor called automatically or something like that). I would suspect that
the cloned instruction is also sticking around with a some Value* (the one he
was planning to return) which triggers your error.

>             }
> 
>         }
> 
>     return NewBB;
>       }

Lastly, you're building a ValueMap, but not using it anywhere. Shouldn't you
be replacing any Value* used in the instructions using the ValueMap? Or is
that handled somewhere else (automatically perhaps?).

Gr.

Matthijs
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20080416/8ff3af6c/attachment.sig>