[LLVMdev] Problems in removing a cloned instruction.

[Prabhat Kumar Saraswat]

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
>
>