Prabhat Kumar Saraswat
2008-Apr-16 10:08 UTC
[LLVMdev] Problems in removing a cloned instruction.
Hi all,
I am trying to write a pass where i am creating a clone of a
function (the only difference being, the new function returns void ,
instead of any value).
I am creating a new Function Type with a void return type (rest being
the same as original function), and using this i am creating a new
function body. Then, i clone the body of the old function into new
function, but when ever i encounter a return instruction with a value,
i am creating a new instruction (return void) and removing the cloned
instruction.
The part of the code that makes these changes is below:
// This function, takes a basic block and clones it to another
identical basic block, the only changes occur for the return
instructions, which are replaced by ret void.
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));
Instruction *NewInst = II->clone();
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);
}
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);
if(ReturnInst *RI = dyn_cast<ReturnInst>(NewInst)) //
deleting the duplicate return
{
cerr << "\n INST : " << *NewInst <<
" Par : " <<
*(NewInst->getParent()) << "\n";
NewInst->eraseFromParent();
}
}
return NewBB;
}
However,functionally pass seems to be working fine (using manual
printfs inside the code), but during the module verification pass, i
get an assert
"opt: Value.cpp:57: virtual llvm::Value::~Value(): Assertion
`use_empty() && "Uses remain when a value is destroyed!"'
failed."
What could be the probem!!
Thanks alot.
Regards
Prabhat
Matthijs Kooijman
2008-Apr-16 12:23 UTC
[LLVMdev] Problems in removing a cloned instruction.
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>
Prabhat Kumar Saraswat
2008-Apr-17 08:59 UTC
[LLVMdev] Problems in removing a cloned instruction.
Thanks Matthijs,
Yes, you were right about the problem when i was deleting the return
instruction, the cloned use of that return instruction was still
there, which was giving the assert.
So, i did as you suggested, i do an early check if the instruction is
a return instruction, if yes, then i create a new (ret void)
instruction and add it to the basic block, if not then i clone the
instruction and do the rest of the thing as before.
It is working now nicely, by doing aforementioned changes.
Thanks again!!
Regards
Prabhat
PS : The new code is below:
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));
// Don't Copy/Clone if the Return Instruction is found
if(const ReturnInst *RI = dyn_cast<ReturnInst>(NwInst))
{
cerr << "\n Return Found : " << *RI<<
"\n";
Instruction *NewRInst = new ReturnInst(); //Create a new return
void instruction
NewBB->getInstList().push_back(NewRInst); // Adding it to the basic bloc
}
else // otherwise
{
Instruction *NewInst = II->clone();
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);
}
if (const AllocaInst *AI = dyn_cast<AllocaInst>(II)) {
if (isa<ConstantInt>(AI->getArraySize()))
hasStaticAllocas = true;
else
hasDynamicAllocas = true;
}
}
return NewBB;
}
On Wed, Apr 16, 2008 at 2:23 PM, Matthijs Kooijman <matthijs at stdin.nl>
wrote:> 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
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.6 (GNU/Linux)
>
> iD8DBQFIBe/Sz0nQ5oovr7wRAq+cAKCpqFDYb6VTMVVbImQs4SrfdfagBwCeLmGZ
> foUgjLPUNtVTMJuDwewCip8> =RpQI
> -----END PGP SIGNATURE-----
>
> _______________________________________________
> LLVM Developers mailing list
> LLVMdev at cs.uiuc.edu http://llvm.cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvmdev
>
>